-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[stdlib] Add optional small buffer optimization to List
, take 2
#2825
base: nightly
Are you sure you want to change the base?
[stdlib] Add optional small buffer optimization to List
, take 2
#2825
Conversation
List
List
, take 2
Any initial perf numbers with this compiler workaround approach? |
I'm not there yet, I still need some more work before being able to run the benchmarks :) I'll ping you with a summary when everything is done. If that is possible to review quickly afterwards, I would be grateful, as I may get a lot of conflicts otherwise. |
Sounds good, thanks! I'll review ASAP once it's ready. |
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
6fee3d9
to
e83c663
Compare
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
!sync |
!sync |
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
!sync |
stdlib/src/collections/list.mojo
Outdated
fn count[T: ComparableCollectionElement](self: List[T], value: T) -> Int: | ||
fn count[ | ||
U: ComparableCollectionElement | ||
](self: Reference[List[U, Self.small_buffer_size], _, _], value: U) -> Int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to use references in those situations otherwise the compiler wasn't happy at all with the extra parameter. The behavior didn't change. Same for other methods like __str__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you file compiler bugs please? Your PRs are definitely hitting a few and I want to make sure they're all captured! Perhaps a comment in the code too referencing such bugs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind filing those bugs but can it be after the PR is merged? Making a minimal reproducible example can sometimes take a few minutes but it can also sometimes take a lot of time. I can make another PRs to add the TODOs later if that's okay with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, totally fine.
Unfortunately this is hitting a nasty compiler crash for internal things with 500+ stack frame trace 😞 . So it won't land today. I'll have to investigate this (won't have time until the work week) and file compiler repros, especially since it's not something repro'ing in the public stdlib alone. |
The parameter name TL;DR: Maybe the parameter should be called |
Also if I'm reading the code right, the "buffer size" is actually the capacity of the list, when it is stored inline. So something like |
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
…nstructor Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
…_branch Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
… remove_copyable_from_collectionelement Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
…lectionelementnew
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
I've hit an issue in the unit test. Not sure if it's a compiler bug or an issue with my code. I'll take a look at it later on. EDIT: I get the strangest behavior. When calling I really have no idea what is happening. I tried printing the addresses at different points in the stack but everything looks correct. I could use some help there. |
!sync |
This PR solves part of #2467
This PR is part of three PRs to read and merge in the following order
List
, take 2 #2825The small buffer behavior
We use
InlineArray
to store up tosmall_buffer_size
elements on the stack.When the user requests a capacity of
N
, ifN <= small_buffer_size
, then the small buffer is used. We make the pointer of theList
point to theInlineArray
. And we set theself.capacity
tosmall_buffer_size
since we can store up tosmall_buffer_size
elements before doing a reallocation.Constructing from existing pointers
When constructing from an existing pointer, to avoid copying the values, the pointer is saved directly in
self.data
without moving the values, even if the size of the pointer is smaller than the buffer. This is to avoid moves for performance reasons. To reallocate the values on the small buffer (if it is big enough) one can simply do a copy of theList
.Alternative considered:
Variant
You might wonder, why not using a
Variant
since we don't need to store thecapacity
and the pointerdata
(and thesize
could be smaller, like anUInt8
) when using the small buffer? This is because by setting the pointer to the stack allocatedInlineArray
, there is no overhead introduced compared to a normal list when doing to following operations: reading, checking bounds, getting the size, getting the capacity. The exact same steps are done, with just different values inside the variables. You might get some speedups when reading the values in the buffer due to cache locality.Checking where are the elements stored
Sometimes we need to know if the
InlineArray
is used or if we allocated data withUnsafePointer.alloc
. When this is the case, we can do the following checkif self.data == self._small_buffer.unsafe_ptr()
. Note that this produces a compiler bug when materializing. See the next PR.Backward compatibility
By default, the small buffer size is
0
. Some@parameter if self.sbo_enabled
here and there make sure that we don't add a single instruction when the small buffer size is null. So the default behavior ofList
doesn't change at all.A notable difference is that
List[Int]
now means a list with no buffer optimization. UsingList[Int, _]
can be used in a signature to signify that the function works with any small buffer sizes.Unit tests
I take all the tests in
test_list.mojo
and run them with 10 different small buffer sizes. I also sometimes mix buffer sizes to make sure that methods likeextends
work with different SBO sizes.I added a materialization test. But it only works with a SBO size of 0 due to the following compiler bug: #2637
Since the bug won't be solved anytime soon, I made #2826 as a workaround