[feat] Make online and offline memory use vectors of pages instead of hashmaps #1224
Conversation
Golovanov399
force-pushed
the
feat/offline-memory-paged-vec
branch
from
6ca9442 to
5070149
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Golovanov399
changed the title
Golovanov399
changed the title
This comment has been minimized.
This comment has been minimized.
| let label = pointer / CHUNK as u32; | ||
| assert!(address_space - as_offset < (1 << as_height)); | ||
| assert!(label < (1 << address_height)); | ||
| if initial_memory.get(&(address_space, pointer)) != Some(value) { | ||
| assert!(pointer < ((CHUNK << address_height).div_ceil(PAGE_SIZE) * PAGE_SIZE) as u32); |
There was a problem hiding this comment.
Isn't the bound just pointer < (CHUNK << address_height)? The boundary chip won't support pointer outside this range.
There was a problem hiding this comment.
Unless pointer is from the last page that was padded to PAGE_SIZE elements and therefore it exceeds the supposed memory
There was a problem hiding this comment.
But that would still be an invalid pointer?
There was a problem hiding this comment.
If address_height is 27 and CHUNK is 8, then the max addressable cell is 2^30 - 1. If I understand correctly, this assertion might allow me to address into 2^30 or higher, if the PAGE_SIZE doesn't evenly divide 2^30. From the perspective of PagedVec, that's a fine index. But from the perspective of memory, that's not a valid pointer.
Or what am I missing?
There was a problem hiding this comment.
Yes, but how does it break anything here?
There was a problem hiding this comment.
This is a test, so I'm not really that concerned. But the point of the assertion is to assert that pointer is a valid pointer. And valid pointers are in the range 0..CHUNK << address_height and, in particular, have nothing to do with the PAGE_SIZE of the underlying PagedVec.
| let block = self | ||
| .block_data | ||
| .entry((address_space, pointer + i)) | ||
| .or_insert_with(|| Self::initial_block_data(pointer + i, self.initial_block_size)); | ||
| .get_mut(&(address_space, pointer + i)) |
There was a problem hiding this comment.
Ideally we would do get_mut on line 336 so we don't need to do it here, but I guess this be problematic for the borrow checker. We should find a way to do this without so many accesses though.
| let result = page[range.start - page_start..range.end - page_start].to_vec(); | ||
| for (j, value) in range.zip(values.into_iter()) { | ||
| page[j - page_start] = value.clone(); | ||
| } |
There was a problem hiding this comment.
Can't we do copy_from_slice or something?
Co-authored-by: Zach Langley <[email protected]>
This comment has been minimized.
This comment has been minimized.
…/openvm into feat/offline-memory-paged-vec
…emory::new`" This reverts commit 0db1666.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Commit: 8ce871d |
This resolves INT-2985 and INT-2984.
Instead of
FxHashMapfor memory, we now use the new structurePageVecwhich isVec<Option<Vec<T>>>, where memory is split into pages of fixed size and we create the whole page when the element is not touched yet, and therefore it should give us faster access to elements.In reality, this depends on the page size, on the queries locality and probably something else. In particular, for regex, execute time and trace gen time reduced by 12%, but for other benchmarks the results are different.
Among other dependent changes, the default
as_heightis now not 29 but 3, otherwise we would have to create half a billionPageVecs every time (and we don't need more than 8 address spaces anyway).