Issue#733 reduce compilation overhead#787
Conversation
|
OS:ubuntu-24.04 |
Signed-off-by: Farhan Saif <fsaif@uic.edu>
Signed-off-by: Farhan Saif <fsaif@uic.edu>
Signed-off-by: Farhan Saif <fsaif@uic.edu>
overlorde
force-pushed
the
gh733-reduce-compilation-overhead
branch
from
976ec26 to
30cab20
Compare
|
OS:ubuntu-24.04 |
angelhof
left a comment
angelhof
left a comment
There was a problem hiding this comment.
Great work on these changes! Can you fix/respond to my points? In particular I would really like to avoid not using the normal Python object initialization. There must be a way to avoid the deepcopy there right?
| @@ -544,8 +555,8 @@ def to_ast(self, drain_streams) -> "list[AstNode]": | |||
| asts.append(assignment) | |||
|
|
|||
| ## TODO: Ideally we would like to make them as typed nodes already | |||
There was a problem hiding this comment.
Remove this comment and the solved now that it is OK :)
| operand_list.extend(out_ids) | ||
| access_map = {output_id: make_stream_output() for output_id in out_ids} | ||
| access_map[input_id] = make_stream_input() | ||
| """ |
There was a problem hiding this comment.
Completely remove this code instead of commenting it out! (similarly in all other places!
| """ | ||
|
|
||
|
|
||
| # Skip __init__ to avoid its deepcopy; inputs are freshly constructed here. |
There was a problem hiding this comment.
Is there any other way to avoid the deepcopy without avoiding the standard init? This seems a bit janky to me to not use Python's normal object initialization. There must be a way to do this
There was a problem hiding this comment.
@angelhof one way on top of my mind is runtime modification of the __init__ , replacing it with fast_init in pash. The existing initialization codes around the pash repo would stay the same. To have a more stable fix, need to go into the pash annotations repo, but the scope becomes larger then, and side effects needs to be checked. Runtime modification / monkeypatching seems like a way where we can use pythons normal object initialization with minimal code changes. I am adding modifications in a while.
There was a problem hiding this comment.
I think the proper fix requires modifying the pash annotations repo, which keeps the scope lower than bypassing Python's init mechanism :) I think we should only accept a fix that requires bypassing init if the gains are on the critical path and very very significant (compilation is not on the critical path, so improving it but making maintenance harder is not worth it IMO).
There was a problem hiding this comment.
nfa-regex.sh — compile time (--dry_run_compiler, 3-run avg - with _fast_init)
| Width | Original (ms) | PR (ms) | Speedup |
|---|---|---|---|
| 2 | 42.33 | 6.12 | 6.92x |
| 16 | 214.51 | 5.92 | 36.23x |
| 64 | 827.98 | 6.95 | 119.13x |
| 200 | 2615.91 | 7.32 | 357.37x |
The speedup is way more than before because of the removal of deepcopy from _init_ - bypassing with _fast_init_ - this time its global cause all instances of __init__ was modified during start/runtime. Same modification on pash annotations repo should also give the same result. As per the last comment, proper fix would be modifying the pash annotations repo, which I agree.
There was a problem hiding this comment.
reverted back to the original init - with shasta typed objects, comments are addressed. Need an integrated test to check both.
|
OS:ubuntu-24.04 |
Signed-off-by: Farhan Saif <fsaif@uic.edu>
|
Back to original |
|
OS:ubuntu-24.04 |
|
Great, do we get any speedups now? If not, what is the expected path to get back the deepcopy speedup? What needs to happen in the annotation repo? |
Yes, we still get speedups from converting to shasta typed objects, below are the ones: Script | Width | Orig | PR | Speedup The deepcopy speedup is more. |
2 participants
Hello All, I am trying to solve this issue #733 , please let me know about the modifications i need to do / feedbacks.
I tried to sovle ir.py:546 comments through directly constructing shasta types and bypassing deepcopy to gain some performance boost.
compile_ir()incompilation_server.pyto bottlenecks - round tripsir.py:547,ir_to_ast.py:114,ir_to_ast.py:151make_kv("Tag", [...])dict construction +to_ast_node(...)round-trips with direct shasta typed constructorsCommandInvocationWithIOVars.__init__deepcopy at 8 call sites via__new__around ~100 ms reduction on top of shasta typenfa-regexdrops from 2675ms to 421ms), andevaluation/tests/test_evaluation_scripts.shpasses 100/100.__init__is safe — no caller mutates these containers after passing them in. Proper fix is to remove thedeepcopyinpash-annotationsitself.speedup gained for nfa-regex.sh
nfa-regex.sh — compile time (
--dry_run_compiler, 3-run avg)