split stack #60
split stack #60
Conversation
|
In order to not make @eamsden navigate through a huge, constantly changing mess, here's how I plan to iterate: Aspirationally, push one batch of commits every day or two, and don't push more commits until Ed has time to look them over and leave feedback. Only include functions with a So I'm marking 0b4029a as my last commit I'll be pushing from this batch. Feedback I'm hoping to get:
|
|
Another batch of commits. I'm mostly looking for comments on |
|
Based on what we talked about, it seems we do actually want a 3rd pointer in It is safe to ignore the |
|
@eamsden Here's the new I'm feeling pretty sure that this is mostly right, so I'm going to move on to |
|
@eamsden here's my first attempt at It may be evident that I still have some confusion over Rust's raw pointer interface (you can see me casting the same pointer to both
I haven't thought through how the memory looks at the end. Should we be expecting that all allocations made for this have been reclaimed? Basically I'm not sure which of these two lines is right |
|
I rushed through https://github.com/urbit/ares/tree/jon/stack-split-mem-rewrite-gogo |
|
Here's my extremely shameful debugging branch: https://github.com/urbit/ares/tree/jon/stack-split-mem-rewrite-gogo-debug So I think I have working implementations of I've been littering printfs everywhere trying to find the culprit. The
So, it looks like the @eamsden anything sticking out that looks strange? |
|
Ok I just noticed that Strange that it seems to get through a fair number of steps before this happens (including at least one similar looking sequence). But that should also narrow it down somewhat. That probably rules out something as simple as the |
|
I added another printf to |
|
So I noticed another thing this morning that probably isn't related but I'll mention anyways - the However, the But - it also looks like the |
|
OK, got it working by just adding 1000 extra slots to every stack frame. Probably this means that the allocation or lightweight stack is destroying the frames somewhere, or there's something off about slot pointer arithmetic. EDIT: I had swapped |
|
The last batch of commits cleans things up a bit. I think the main trouble that's left is surrounding the behavior preparing to pop a stack frame. When we evacuate allocations in the current frame to the previous frame, we destroy the FP-relative slots and lightweight stack. To save the pointers to the previous frame, we call Then, in order to do noun traversal for e.g This makes things pretty confusing for the programmer. They have to know whether they're working before or after |
|
Ready for review. A few things I wish to draw attention to:
|
|
Having done enough of #66 to know how the split stack interface is going to be used for it, at this point the only thing that's gone on the lightweight stack are nouns. If this is always the case, then the weirdness I'm worried about with respect to needing to know the type of what's on top of the stack in order to pop it wouldn't actually be a problem. |
|
So the choice of where the |
|
@ashelkovnykov Thanks for your comments, I'll get to them in the morning! |
I was mistaken about this. We have nouns, |
|
I believe I have addressed all comments. Would you please give it another look over @ashelkovnykov @eamsden |
|
What's the deal with squashing commits? I've only done it sparingly, such as when I'm really confused and need to test out a lot of different paths and stashing isn't enough so I use WIP commits. Many (most?) of the commits here have commit messages explaining their rationale. Squashing them would create a gigantic confusing commit message and thus make the commit history a lot less useful IMO. If that's just the style for the Ares repo I'll do it, but I think it is a loss. Unless you mean I should go through all the commits and figure out which ones are appropriate to squash together. Which sounds tiresome, but I'd be fine with doing that. |
|
Not sure how to deal with this from CI: https://github.com/urbit/ares/actions/runs/5790048220/job/15692346635 I definitely don't think I should rewrite this using |
I don't actually know what the style is regarding squashing commits for Ares - we should confirm with @eamsden.
I meant squash them all down into one commit, to be courteous of your time, but I do personally prefer the practice of every PR in a commit being a logical, (almost) self-contained grouping of changes.
This is true; I just took a look at them and they're a lot more descriptive than the usual fare one would expect from a PR with nearly 100 commits.
I disagree. We care about the changes and we care about the discussion around the changes, but we don't care about intermediate work. Any important discoveries or notes made during the process should be documented somewhere far more discoverable than the commit history. 87 commits is massively excessive for what comes down to ~1000 modified lines. If I want to browse a history of the changes, I don't want to have to scroll past 87 commits that are all introducing the same one feature. If I'm doing a git-bisect, I don't want to waste hops because 87 commits are all introducing one feature. Regardless, the most important thing here is the changes themselves, so I'll go with whatever decision @eamsden makes regarding git + GitHub etiquette. |
@drbeefsupreme I think our options are:
|
|
On squashing: you can always edit the commit messages. Squash together into one or 2 commits of working code On CI: that particular linter bit is fine to suppress, ifs with comparators are good and fine |
|
I was just hiding behind laziness on making more than one squash commit. I don't actually know how to do anything more complicated than that yet but I'll figure it out and squash this into a few commits, and suppress the warning. |
see docs/stack.md for information about this change. split: replace Polarity enum with pointer check we can check polarity with West === FP < AP, East === FP > AP. we panic when the pointers are equal. split: pre_copy step before preserve() calls we need to save the stored frame_pointer and alloc_pointer before preserve calls, which will otherwise overwrite them. split: copy_west(), now with stack_pointer! when copying, we can forget the lightweight stack in the current frame and start a new one adjacent to from-space used for traversing nouns in from-space. split: working split stack this commit works with hurray.jam and decrement.jam. there's at least one thing left: I don't think the implementation of hamt::preserve() is correct, but hurray.jam and decrement.jam don't use it so it doesn't detect it. the comment there explains whats wrong. I also need to double check copy_pma() still. This is the commit message #52: split: hamt::preserve() and free_alloc hamt::preserve() needs to allocate in the current frame after we've already said we can no longer allocate (once pre_copy() has been called). so does e.g. copy_west(), since it moves the lightweight stack into this same area. we can unify this interface but I want to explore the ergonomics of making this free allocation stuff usable for the programmer without having to think about whether or not pre_copy() has already been called. split: add boolean for pre_copy() alternative: use an enum to say whether the lightweight stack grows westward or eastward, and that the reserved slots for the previous frame's pointers are adjacent to the frame_pointer or alloc_pointer. not sure which is the better choice yet. split: panic when allocating at the wrong time when pc == false, we aren't in copying mode (basically when we're copying allocations to the previous frame to get ready to pop the frame). therefore we should panic if we try to allocate in the previous frame. similarly, when pc == true, we're no longer allowed to allocate in the current frame, but we can allocate in the previous frame. therefore we should panic if we try to allocate in the current frame. split: use prev_stack_pointer_pointer() we'd like the programmer to not have to think about whether the frame is in copying mode or not, so every time we want the previous frame's stack pointer we should just called prev_stack_pointer_pointer() instead of trying to remember if we need slot_pointer or free_slot. we still use slot_pointer and free_slot for pushing and popping stack frames, since we're in the midst of changing the pointers that would determine where prev_stack_pointer_pointer() would look. split: polarity of lw stack changes in pre_copy It was waiting until copy_west/copy_east to do this - this makes it more clear that the lightweight stack is different after pre_copy() is called split: simplify is_west asking this to panic in case of equality is assigning it too much responsibility. it should just say whether or not a frame is west. This is the commit message #76: split: copy_pma isn't in cleanup mode we don't need to move the lightweight stack since we're not allocating in a previous frame, but into the PMA split: revise pre_copy() usage pre_copy() is no longer public. instead, it is called at the start of any NockStack method which could potentially be the first function called when getting ready to pop a stack frame (preserve() and functions that allocate in the previous frame). main: trailing bytes of cued jamfile are zeroes this was already the case, but it was doing so incorrectly. write_bytes(dest.add(word_len as usize - 1), 0, 8) copies 64 bytes of zeroes starting at the last word of the indirect atom. this went unnoticed before split stack since allocation happened at the west end of the memory arena. now that allocation starts at the east end, this was causing an overflow. so the correct line is write_bytes(dest.add(word_len as usize - 1), 0, 1) split: memory in NockStack::new not mutable it was before split stack, now its unnecessary and throws a compiler warning. im confused as to why it was ever there at all, though
drbeefsupreme
force-pushed
the
jon/stack-split-mem-rewrite
branch
from
a6143e8
to
81a7eb7
Compare
|
Ok I squashed all the commits and kept the useful sounding commit messages, but it doesn't look like there's a way to see what code they're referring to? Is there any way to maintain that or is it just lost unless I single out the commits with useful messages and squash everything in between them? Sorry this process is confusing for me - what the right convention is and what the results will look like when I try a new process is unclear to me. |
In urbit we basically do commit-per-feature, so once commit saying this introduces the split-stack is fine |
Addresses #50
Following @eamsden's advice and a couple attempts so far at refactoring the old version, I am rewriting
mem.rsfrom scratch since this change touches most of the file.