Implement indexed map access (incubating feature) #2068
Conversation
This is why unordered map is used. We maintain the ordering by keeping the keys ordered in a vector on insert. |
|
Can you elaborate why you need the It looks like |
| adjust_after_pushing(p); | ||
| return *this; | ||
| } | ||
| Expression* key(size_t i) { | ||
| return ordered.at(i * 2); |
|
@xzyfer Can you elaborate where "we maintain the ordering by keeping the keys ordered in a vector on insert". I did not find any vector where we "keep" the insertion order, that is exactly why I have added "deque". Edit. Guess I should have looked more closely. Found it sigh |
|
FWIW I don't get email updates when editing comments. It's worth just posting a new comment so I'm not email back out of context replies. Timezones 😞 |
|
I also added the values to the "vector", since it should make it faster to iterate over values then by fetching each key from the hash map itself. It may also be more effecient to use deque instead of a vector. We can also easily implement optimal iterators over values and keys. To access keys or values you just have to multiply the index by two (and adding 1 for values). |
|
The extra memory doesn't seem worth it. Is there a use case for iterating over values? |
|
The overwhelming majority of map usage in Sass is direct key look up. We should optimise for that use case above all else. |
|
I agree that direct key look up is the most important one, but I would like to guarantee that it doesn't degrade too much in worst case scenario. I need to make some performance assessments first, to determine if this is the case with repeated key access vs. random access via deque. IMO the memory overhead is very little compared to the actual data that is stored (4 to 8 additional bytes per item, so 1MB should fit 128k items on x64 arch). We do not have many places where this is currently used. On a first glimpse it seems to only be the case in map destructors and we could probably refactor that to use the iterator (no need to destroy in insert order). On the other hand there are the C-API functions that basically have to traverse a map in this way if they need to do so. It seems that i.e. But I think it probably makes sense to use "deque" instead of vector in "Hashed", as appending stuff is cheaper for deque and random index access is also O(1). There are always pros and cons. |
|
I'm 👍 for make |
|
The tradeoff in code complexity is also something to consider. I'll take slight less efficient, user friendly, maintainable code instead of overly complicated, over optimized code any day. |
|
With that said adopting deque isn't necessary to fix the issue at hand. I'd |
Fixes #1930
Was even simpler than anticipated. Had to add a
dequeobject to the hash map implementation. I also tried to switch to astd::map, but it doesn't seem to support a custom hasher and only works with compare operator (which is not good, since we cannot really compare different types of ast nodes). It wouldn't had much benefit anyway (we would only get an ordered iterator over all pairs). As always with hash maps, it more of a guess which implementation would lead to the best performance.IMO this implementation is pretty close to being optimal (in terms of performance). Random/Indexed access is constant
O(1)due to the use ofdeque. I didn't put too much thought into the API, but it should be straight forward to use. You can fetch key or value by index. As I already wrote in other threads, the list/map interfaces/types/classes should be refactored and unified at some point (basically hiding all the logic currently implemented in functions.cpp and elsewhere).If you can point me to other functions that allow random/indexed access to maps, please add them in this thread. Will see if I can also implement them. And yes, this is still missing proper spec test coverage.