Stop unneccessary copies of class_hierarchyt #2971
Conversation
JohnDumbell
requested review from
cesaro,
cristina-david,
kroening,
mgudemann,
NathanJPhillips,
peterschrammel,
pkesseli,
smowton,
tautschnig and
thk123
as code owners
| @@ -68,7 +68,7 @@ resolve_inherited_componentt::inherited_componentt | |||
| } | |||
|
|
|||
| const class_hierarchyt::idst &parents= | |||
| class_hierarchy.class_map[current_class].parents; | |||
| class_hierarchy.class_map.at(current_class).parents; | |||
There was a problem hiding this comment.
Maybe add a precondition that checks that it is there?
There was a problem hiding this comment.
While I never had an exception thrown, in the spirit of keeping the code functionally the same it might be better to do a find on the map, then only do anything more if that succeeds.
Think that would be prudent, or leave it as-is and add the precondition?
There was a problem hiding this comment.
Passed Diffblue compatibility checks (cbmc commit: 2fbad7e).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85047439
There was a problem hiding this comment.
We don't use the naming convention 'owned'/'borrowed', and I'd be reluctant to introduce it here as a special case.
What we do to prevent copying of potentially large data structures is to delete the copy constructor and assignment operator; I'd do the same for the class hierarchy.
|
@kroening if not owned/borrowed what naming do you prefer? |
There was a problem hiding this comment.
The PR comment says:
If the symbol table is large enough all the copies cause a significant slowdown to the transformation of Java -> GOTO.
This just adds references to a few places where it seems they were missed.
This info is missing from the commit message, but also neither has actual data points (what is "significant" - 5 seconds, 10 minutes, 2 days; what's the impact of adding references?).
| @@ -70,7 +70,8 @@ class resolve_inherited_componentt | |||
| const irep_idt &component_name, | |||
| const irep_idt &user_class_name); | |||
|
|
|||
| class_hierarchyt class_hierarchy; | |||
| const class_hierarchyt &class_hierarchy; | |||
| class_hierarchyt owned_class_hierarchy; | |||
There was a problem hiding this comment.
There already has been a discussion about terminology, which we may be able to steer to some conclusion if someone could explain what the difference between those two actually is? Why can't they be the same?
There was a problem hiding this comment.
This was purely to get around the fact that reference fields require some value on construction and that class_hierarchyt gets populated by a () operator, so needs something to exist before passing a symbol table too it.
Edit: actually don't think my suggestion would work, so any others would be appreciated.
There was a problem hiding this comment.
Hmm, is the constructor that take a single argument (just the symbol table) actually ever called!? Maybe the simple fix is to get rid of that constructor?
There was a problem hiding this comment.
CLion doesn't think so, but is this something other people can/would be using? Otherwise I'm good with removing it.
There was a problem hiding this comment.
If there are any such users, then they just need to create the class hierarchy themselves and pass it as second argument.
Unless someone steps up immediately, I'd just go with removing it. Commits can always be reverted, this isn't a one-way door.
JohnDumbell
force-pushed
the
jd/bugfix/unnecessary_class_hierarchy_copy
branch
from
2fbad7e to
65b81c5
Compare
|
I've updated with all the comments given so far. I've also removed the constructor from resolve_inherited_componentt as we're not using it, which solves the issue about duplicated fields and naming convention. In regards to actual data around improvements: I was running with a symbol table of around 220,000, and with this change it went from hours to 2-3 minutes to parse and then attempt to generate GOTO. I can't provide exactly how long the pre-change took to get to the same point because after it took around an hour I just set about looking to see if there were any easy ways to speed it up. I can give exacts if required, but it'll probably take a day to get. There should be no logical impacts around it becoming a reference vs a copy, the code using it only did look-ups from what I could see. |
In cases where the symbol table was large (220,000) continuous copies of class_hierarcht across a few method/object boundaries were causing noticable slowdown. This just adds in a few references where they seem to have been accidentally missed.
JohnDumbell
force-pushed
the
jd/bugfix/unnecessary_class_hierarchy_copy
branch
from
65b81c5 to
00550cd
Compare
There was a problem hiding this comment.
This PR failed Diffblue compatibility checks (cbmc commit: 65b81c5).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85296801
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
|
Above failure looks like you've recently rebase - hopefully Joelbot will post some positive news shortly (given this is an interface change, could you wait until you do get a positive response). |
There was a problem hiding this comment.
Passed Diffblue compatibility checks (cbmc commit: 00550cd).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85301103
|
@JohnDumbell Please remember that performance story for future company training sessions! |
If the symbol table is large enough all the copies cause a significant slowdown to the transformation of Java -> GOTO.
This just adds references to a few places where it seems they were missed.