-
-
Notifications
You must be signed in to change notification settings - Fork 947
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
[core] Implement SmallVector using std::vector #7165
Conversation
I'd be interested to see if there's any significant differences in compile and/or run time for this method |
Running Old:
New:
So 11% slower compilation, not surprising seeing as this is an intermediate shim. I expect this to improve once SmallVector is removed completely, as only one template will be instantiated per use, rather than two. Run time performance would hopefully be better, owing to exponential growth of storage rather than linear growth. The loss of realloc may counter that to some degree. But if trivially relocatable types (P1144R1) are approved for C++20 then that would be resolved automatically. Also explicit template instantiation should yield good results. |
Run-time performance is far more relevant that compile-time. |
Is there a benchmark for that? |
you used to be able to run a non-gui benchmark with "-v null:ticks=1000"
|
Running for 10000 ticks I got: Real -2.1% So a boost in all categories!
|
Possibly it might be good to run for more ticks; comparing two results based on runs of ~1 second means you are comparing heavily in the noise ;) Longer runtimes will give more comparable results :) |
It seems you forgot to replace some Clear() instances (as implied by the CI failure) |
cf52201
to
38cf2f2
Compare
And now it fails on Length() Edit: missed the forced push ;) |
2b3a8a8
to
f319524
Compare
|
This was discussed in the previous version of the PR. I thought it made more sense to do the change in individual commits, but the same PR |
EDIT: Reviewing individual commits is a must for this PR! |
Maybe, maybe not. I figured that reviewing method a -> method b (lots of single line changes) was easier to review than object a -> object b (lots of bigger changes), hence the move to make SmallVector a subclass of std::vector |
1adf753
to
6c6f630
Compare
f526d6b
to
862c3a8
Compare
…o[Free|Delete]SmallVector
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'm happy with this, as it stands. There may be minor tweaks here and there but they can be done in master.
…ng types. Const and non-const Find() have different return types.
Const and non-const Find() have different return types.
…et relative' button crashes the game. The 'offs_start_map' is a 'SmallMap', so its own 'Erase' function should be called instead of the underlying vector's 'erase' function. And fix a "typo". :)
…ng types. Const and non-const Find() have different return types.
This the starting point for removing SmallVector by making SmallVector an adapter type.
This replaces #6817.
The public and protected interface to SmallVector are unchanged
SmallVector now requires that items be default constructible
This isn't an issue since some contained items were previously created
uninitialized.
Temporary default constructors are added to the following structs
Where vector is required, transition immediately to std::vector
to avoid returning proxy object references.