mycpp: only emit StackRoots if Collect() possbile #1963
Conversation
|
This also improves many of the cachegrind benchmarks by a good margin: http://travis-ci.oilshell.org/github-jobs/6915/benchmarks2.wwz/_tmp/gc-cachegrind/index.html |
melvinw
force-pushed
the
call-graph
branch
2 times, most recently
from
346cbf5
to
ecff5d4
Compare
This reduces the number of stack roots by almost 10x.
|
Thanks for looking into this, it's pretty awesome that it makes us clearly faster than bash on several benchmarks/compute, in addition to Python configure speedups http://travis-ci.oilshell.org/github-jobs/6921/benchmarks.wwz/_tmp/compute/index.html Also it seems to be a pretty small amount of code It seems like we clearly need something like this -- it's a major improvement. And I'm glad you got past some of the "smells" like module aliases I am going to poke at the tests a bit, I think there are some more cases we could add Also I am dying to get rid of the And maybe it seemed too onerous Since this adds some extra logic in the decl pass, it might be worth looking into again But more important is if there are any cases where this could cause a runtime error, i.e. with new code patterns from contributors I would also like to make a That might be possible without a full "yaks" rewrite ... |
It segfaults Let's see if it segfaults without this rooting optimization
|
Hm I started hacking on some more tests and was able to break it I pulled the new tests into a branch based off master, and ran them, and they pass https://github.com/oilshell/oil/tree/test-call-graph I didn't look into the root cause, but it appears this change causes a problem. The test case is pretty simple -- it creates a big list, deletes some items, and then collects periodically in a loop ASAN flags the error, so you can just run the test_root_call_graph by itself to see the error I didn't add the virtual function case yet, so there may be more I think we need something like this, but it's not urgent and we might want to take some time to make mycpp better (depending on what the bug is) |
|
Nice! I'll take a look at this over the weekend. I'm curious what's getting tickled |
|
BTW this gets rid of about 400 KiB of executable code After - http://travis-ci.oilshell.org/github-jobs/6929/cpp-coverage.wwz/_tmp/metrics/compare-gcc-clang.txt Before: Actually nearly 1 MB for Clang for some reason !! We have a somewhat old version of Clang I think So I hope that we can do this correctly within the existing mycpp, with a few modifications But if not it might be some motivation to rework mycpp a bit |
|
Oh right! I forgot that the stack roots contributed so much to the size of the binary. That's an awesome second result |
|
Aha! I was using the caller's name instead of the callee as the destination of the edge in the trivial case! Apparently we don't have many calls like this that lead to the GC in the spec. The fix added less than 100 roots. |
|
The linter failure from CI is confusing. It's complaining about an unused I don't see it locally either. |
There was a problem hiding this comment.
OK glad it was a simple fix
Can you add a test case with a virtual function? That's what I was in the middle of, but I am reviewing some other PRs now
I believe that is realistic because say SubProgramThunk::Run has a CommandEvaluator and will use it
And it's a virtual function
Also we have virtual functions in vm::_Executor, WordEvaluator, etc.
I think the case we want to catch is that only one of the virtual functions creates a path to mylib.MaybeCollect(), but not all of them
| @@ -11,3 +12,7 @@ def func2(): | |||
|
|
|||
| from testpkg import module1 | |||
| log(module1.CONST1) | |||
|
|
|||
| def func3(): | |||
There was a problem hiding this comment.
Even in the test code I think it's better to be consistent about naming, e.g.
DoesCollect() rather than does_collect()
Func3() rather than func3()
Sometimes we follow MyPy naming but here there is no real reason to
|
Also I wonder if anything can be moved to I added our virtual function heuristic there -- we need to add So the Virtual state has some clear objects and methods, and we know what the LIMITATIONS of the heuristic are The reason is mainly because I expect we need to clean up and most likely gradually rewrite mycpp. So having code outside of that big pass file is less coupled to the MyPy AST (Also if you or anyone else is interested in writing a simple type checker, I think that is possible / in scope, now that we are 100% translated, and know all the idioms we use. This would let us get rid of the pinned version of MyPy I may start a thread about "copying" the MyPy AST into |
|
Hm yeah we talked about callbacks a little, they are the same problem as virtual functions e.g. you could have two different functions with signature However, I think we almost never use this. We almost always prefer a class with subclasses and then a virtual method like I am not sure if we even translate callbacks correctly But is something to think about and probably test. mycpp is still missing test coverage, because we just made it run one program. If we don't support callbacks then we could have Unfortunately right now I don't remember if we do or don't |
melvinw
force-pushed
the
call-graph
branch
3 times, most recently
from
d053201
to
2fb3400
Compare
xxx would be nice to minimize all the flipping between '.' and '::' as scope separators...
This commit adds a library for implementing a simple control flow graph. Right now it doesn't have any meaningful statement annotations, nor is it connected to anything in mycpp. Future commits will build on top of this to generate datalog programs to perform analyses like the call-graph analysis from #1963 or the alias or stack root analysis from #1972.
This commit adds a library for implementing a simple control flow graph. Right now it doesn't have any meaningful statement annotations, nor is it connected to anything in mycpp. Future commits will build on top of this to generate datalog programs to perform analyses like the call-graph analysis from #1963 or the alias or stack root analysis from #1972.
Most instances of
StackRootare unnecessary because they are created in functions where there is no danger of the garbage collector being invoked. This commit changes mycpp to construct a graph of function calls across the entire program and uses it to decide ifStackRoots are necessary for a given function. An directed edgeu -> vmeansucallsv. Mycpp builds up this graph during the declaration pass, and passes it to the definition pass, which uses it to search for a path from the body of a function tomylib.MaybeCollect, only emittingStackRoots if a path is found.This reduces the number of stack roots by almost
10x(EDIT: The new examples caught a bug. This is actually ~5x. The performance impact is about the same, though.) and has a noticeable impact on the wall time of workloads like the CPython configure script (see reports below. ~1-2% or 10-150ms). It's important to note, though that this optimization only considers the static call graph. Many of the remaining functions withStackRoots are functions in theCommandEvaluatorthat are mutually recursive. Reducing overhead from those will require a different approach.stats from master
stats from this branch