-
Notifications
You must be signed in to change notification settings - Fork 511
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
WIP: Port both IdList and List to std::vector #433
Conversation
Does this actually have any benefit? Specifically, take a look at IdList indexing, which seems much more useful. |
The main reason I first did this was to reduce allocations, since that was a big drag on performance when I profiled last year on Windows. Allocation patterns/behavior for performance are fairly tuned in stl implementations to provide the fabled amortized-O(1) performance. I also just get nervous in general at manual memory management of this sort. Certainly would be nice to e.g. use an unordered map here instead, but that's a larger change I hadn't done before. I'm still mostly just trying to clear my patch backlog as I promised to do :) I enjoy working on SolveSpace, nearly to a fault, so I'm trying not to do too much original dev on it right now. |
Do the tests pass after 49a7f86 ? |
@ruevs Tests pass before and after that commit, because there are currently no tests that create groups, only tests for roundtrips. That's a known problem, but adding such tests is not trivial, sadly. |
Eliminates a class of reasons for not using range-for: Adding during iteration. Instead, just add to a new IdList that you merge in after iteration is complete.
The issue before is that much of the code relies on a copy or assignment of IdList being a shallow copy, so using a shared_ptr was the simplest way to preserve that meaning. (Using a Vector directly was unsuccessful due to changed copy semantics. The variety of iteration methods makes a port to shared_ptr<unordered_map> harder and more far-reaching, if nevertheless worthwhile.)
shared_ptr for the same reason we needed it for IdList.
please, compare performance with IdListIndexing https://github.com/Evil-Spirit/solvespace-master/commits/id-list-indexing |
dfb96de
to
3f09eaf
Compare
Closing as idlist was improved in another PR. |
WIP because it doesn't pass the tests. (All but the last two commits pass - those passing commits are in other PRs) Current test results: