SymbolStack: Use hash map instead of array for scopes #573
Conversation
|
Did this just to test it out, I might want to clean up the API a bit but the results look promising |
With |
|
Is there an advantage to using |
|
|
|
Unless I'm misunderstanding your suggestion, does that matter? We don't need to iterate over the keys/values, we only ever do lookups. I thought you were suggesting that in |
|
I meant that the current |
|
Thinking about this some more - given that C allows variable shadowing, how would this work? e.g. if the code being parsed is: int x = 5;
void foo(void) {
float x = 1.0f;
}We encounter the first |
|
Hmm, I think it would probably be possible with a custom context that accounts for the scope index but that can be a future improvement. |
|
Sounds good. When I get a chance I'm going to update it to use a multiarray of hashmaps and use |
ehaas
force-pushed
the
hashmap-symbols
branch
3 times, most recently
from
8651068 to
047890c
Compare
|
Using |
There was a problem hiding this comment.
Using clearRetainingCapacity made no difference in my benchmarks (sqlite3.c and zig2.c) in ReleaseFast, which was surprising to me.
Huh, I would not have expected that, I guess it would be because clearRetainingCapacity memsets the entire capacity of the hash map.
Use whichever version you prefer more but the current one needs some changes.
src/aro/SymbolStack.zig
Outdated
|
|
||
| pub fn deinit(s: *SymbolStack, gpa: Allocator) void { | ||
| s.syms.deinit(gpa); | ||
| std.debug.assert(s.innermost == std.math.maxInt(@TypeOf(s.innermost))); |
There was a problem hiding this comment.
Could you instead change get to use innermost - 1 and remove the wrapping from pop?
There was a problem hiding this comment.
I've been coding on a plane with 2s latency so it's possible I messed up the benchmark; I'll repeat it when I get home.
Updated SymbolStack to remove the wrapping and renamed the field to active_len since I think that more accurately describes its role after the change.
Previously we weren't checking if a name already existed as a different kind of symbol when defining typdefs, and weren't checking if typedefs existed when defining other kinds of symbols which use var scope.
Vexu
force-pushed
the
hashmap-symbols
branch
from
bd4d35b to
304f87e
Compare
|
Ok I ran the benchmarks again: First is the baseline original hashmap implementation. Top set is |
Vexu
force-pushed
the
hashmap-symbols
branch
from
304f87e to
9949e8c
Compare
On a preprocessed
zig2.c(with 1cdf4d5 reverted to prevent a parse error) I get the following:With this branch I get
We have a lot of spurious "non-void function does not return a value" warnings and some incorrect integer overflows but I don't think fixing those will make arocc 10x slower :)
As a future enhancement we could look at reusing scope hash maps instead of allocating and freeing them all the time.
Closes #321