-
Notifications
You must be signed in to change notification settings - Fork 205
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
[Feature request] Expose tokensMap
as public
#1975
Comments
In theory it could be changed to a Map instead of a dictionary at some point. e.g. something like: import { createToken as orgCreateToken } from "chevrotain";
const tokensMap = {}
function createToken(opts) {
const newToken = orgCreateToken.call(null, opts);
tokensMap[opts.name] = newToken
return newToken;
} |
@bd82 Your code is precisely what I am doing. I just would prefer to use internal structures if they are stable. I think you're probably right though that Maps can be slightly faster for lookups than object keys (although V8 and other engine internals are constantly changing). |
FYI, object key lookups in v8 (and most other engines as well) are multiple times faster than map lookups. Noticed that while experimenting with #1793. |
Do you have data / benchmarks to back that up? Why would a structure that is specifically designed for fast mapping ( An initial Google search returns a lot of evidence of maps not being faster in almost every regard, but also much more memory efficient. However, you have to be careful with testing performance (I guess?) because thousands of repeated object writes / lookups may be optimized with inline caching to profile and return the same internal shape for an object, which would not happen as frequently with a real-world case with limited writes / reads. So a test of "object vs. map" can show results that are actually in-applicable (unless, I think, you use a fresh set of keys or an isolated / fresh v8 process per single run of a single test case). But I'm happy to see more data, if you have it, because I think this is something that is not 100% clear to most JS developers. It seems like a |
@msujew Here is what a V8 developer said in response to some on this issue who said:
The V8 developer said:
So that's from two sources: one is a v8 developer, and another is someone profiling with some awareness of v8 internals. Both with the same takeaways:
But, again, if you have evidence that contradicts these results, it would probably be worth sharing! |
One more point: something like an IToken object in Chevrotain is probably best to stay as an object map (or, alternatively, class instances) because it is a fixed shape and that shape is re-used a bunch of times. So that could benefit from inline caching where individual maps could not. So I think you have to think about what is the "map" you're trying to store and how many "copies" of that map shape you might consume. But this gets into nuance where a JS engine dev would know better. |
There are a number of useful class properties / methods on the BaseParser / CstParser that aren't exposed via TypeScript, so it's hard to know if they're stable / safe to use. One of those is
tokensMap
. I'd been creating this exact type of map for use when parsing, so I was surprised to find the same structure on the instance itself when debugging. I'd like to eliminate creating this map, but I'm not sure iftokensMap
is safe to use.The text was updated successfully, but these errors were encountered: