Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

u3: adjust free-list sizes #539

Merged
merged 6 commits into from
Nov 10, 2023
Merged

u3: adjust free-list sizes #539

merged 6 commits into from
Nov 10, 2023

Conversation

joemfb
Copy link
Member

@joemfb joemfb commented Oct 12, 2023

This PR dedicates slot 0 to cells (u3a_minimum), and aligns subsequent slots on power-of-two boundaries. Previously, slot 0 was unused, and subsequent slots were aligned near power-of-two boundaries. For example:

words old slot new slot
6 1 0
31 3 2
61 4 3
62 4 3
63 4 3
121 5 4
122 5 4
123 5 4
124 5 4
125 5 4
126 5 4
127 5 4

The first issue dates to urbit/urbit#987. Slot 0 was reserved for sizes less than 8, which meant that 7-word allocations on the home road were incredibly slow (as they traversed a free list full of 6 word allocations). The second issue has always been the case in this allocator, due to the way that the size was rounded up on each iteration of the "power-of-two" sizing loop.

This PR does not address the infamous size-bumping logic in the allocator:

vere/pkg/noun/allocate.c

Lines 453 to 467 in 9bdc1af

/* XX: this logic is totally bizarre, but preserve it.
**
** This means we use the next size bigger instead of the "correct"
** size. For example, a 20 word allocation will be freed into free
** list 2 but will be allocated from free list 3.
**
** This is important to preserve because the sequential search may be
** very slow. On a real-world task involving many compilations,
** removing this line made this function appear in ~80% of samples.
**
** For reference, this was added in cgyarvin/urbit ffed9e748d8f6c.
*/
if ( (sel_w != 0) && (sel_w != u3a_fbox_no - 1) ) {
sel_w += 1;
}

I'm been tempted to disable that behavior on the home-road (to further reduce fragmentation), but I'm not convinced that won't still run into pathological performance issues. The best approach might be to start searching the "proper" free list, but bound the number of iterations before bumping. Either change can be made at any time, without migrations.

This PR does requires a migration to move free space into the now-appropriate free-list. It includes a trivial, always on migration, which should be refactored into #508 when appropriate.

This PR helps somewhat with urbit/urbit#6805, as it reduces home-road fragmentation.

@belisarius222
Copy link
Contributor

It would be great to do some before-and-after benchmarking for this PR. Could measure things like initial boot and handling an Ames message.

@joemfb
Copy link
Member Author

joemfb commented Nov 9, 2023

I've updated this to only migrate free lists during the v2-v3 migration from #508.

@pkova I'm not sure if people have already been running #508 -- if they have been, the migration won't rerun and we'll have to do something else.

@belisarius222 I ran the jam/cue benchmarks on develop and on this branch. there's a minor speedup on some benchmarks, maybe 2-10%. keep in mind that these are imprecise and use wall-clock time over many iterations, and that only some allocate cells on the home-road (the others aren't that relevant). results here: https://gist.github.com/joemfb/3a3441d82b00d3227c50350a4bb53832

@joemfb
Copy link
Member Author

joemfb commented Nov 10, 2023

On second thought, any piers that have already migrated should just be able to pack or meld to reclaim from there now incorrectly-sized free lists. Until they do so, some allocations will be a little bit slower, but there shouldn't be any other problems.

@pkova pkova merged commit 6f63ab0 into develop Nov 10, 2023
5 checks passed
@pkova pkova deleted the jb/free-list-size branch November 10, 2023 16:44
pkova added a commit that referenced this pull request Nov 13, 2023
... early migration prerelease

In other words, my comment at
#539 (comment) was
wrong, the couple of ships that are affected cannot just meld because
the loom-sane assertion runs too early.
joemfb added a commit that referenced this pull request Nov 15, 2023
The last minute migration change to #539 was just wrong. #551 is a
functional workaround, but the change to the v3 migration itself also
needs to be removed. We regret the error. (This can be fixed for real in
v4 migration.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants