[Util] clean up flags #10958
[Util] clean up flags #10958
Conversation
kabdelhak
commented
Jul 12, 2023
kabdelhak
commented
- remove bad to maintain index structure in flags. everything works with unordered maps now
- flags can be ordered in any way, no consecutive indices to maintain
- remove some unnecessary code and clean up
- no functional changes
- remove bad to maintain index structure in flags. everything works with unordered maps now - flags can be ordered in any way, no consecutive indices to maintain - remove some unnecessary code and clean up - no functional changes
kabdelhak
added
the
COMP/OMC/MetaModelica
| else | ||
| // this should not happen, but in case its not set, just use default | ||
| outValue := inFlag.defaultValue; | ||
| end if; |
There was a problem hiding this comment.
It would be better to use UnorderedMap.get and then check if it's SOME or not, instead of doing contains followed by getSafe which effectively means looking up (and hashing) the key twice.
There was a problem hiding this comment.
Thanks, should probably do that in multiple places!
|
Looks good in theory, but we need to make sure it doesn't cause any performance issues. Looking up flags is already somewhat slow, and this will make it even slower. We usually try to avoid having flag queries in the hot path, but this could potentially reveal new such issues. |
I don't really understand how it is slow, isn't it just accessing an array? From my understanding an unordered map just has the hashing overhead. |
It has to check if the flags are initialized, get the flags from the global root, and then finally fetch the correct flag. So a lot of MetaModelica overhead. It's not that slow, but we've had issues before with flags being accessed too much and slowing things down. So the change itself is fine, but it would be a good idea to just do some performance tests on e.g. some large models just to make sure we don't still have any excessive flag accesses anywhere.
The bootstrapped compiler probably doesn't support all the new features used by the unordered maps, I don't think it's been updated in quite some time. So you probably need to update the bootstrapping sources. Good luck with that 😐 |
I am updating the bootstrapping sources... seems like a headache with all the dependencies but probably worth it. I will also do some time consumption tests to see if your assumptions are true. |
- add unordered map (since flags now depend on it) - add vector (since unordered map depends on it)
|
|
