Skip to content
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

use li->roots instead of module constant table #14656

Merged
merged 1 commit into from
Jan 13, 2016
Merged

Conversation

JeffBezanson
Copy link
Sponsor Member

Looking at #14556, I experimented with (1) disabling AST compression, and (2) returning constant tables to per-function instead of per-module. Here is the data:

sysimg tests peak mem
master 29.49MB 1788 s 975MB
no compression 30.07MB 1787 s 1669MB
li->roots 29.83MB 1547 s 911MB

With the new GC, AST compression is not as crucial as it used to be. Performance is on par (some things were faster and some were slower) but memory use is way higher. However using function-local constant tables wins hands-down by removing most of the O(n^2) constant lookup while retaining the benefits of compression. The only benefit of per-module constant tables is system image size, but not by much. Also I suspect if the common symbols lists were recomputed for the no-compression case its system image would improve a lot.

The subarray test accounts for almost all of the improvement; it got ~100s faster. In that test the constant tables are huge, and consist almost entirely of types. In the no-compression case the subarray test is also 100s faster than master.

So for now I recommend ripping out the per-module constant table since it's an easy win. Long term, we should get rid of AST compression by addressing memory use in that case. Ideas there:

  • Replace all remaining Exprs in our IR with more compact specialized objects.
  • Delete more ASTs. There can be heuristics for ASTs not likely to be needed. For example specialized ASTs too big to be inlined (and not declared inline).
  • Or... switch to a bytecode representation. Most efficient but difficult to do.

@simonster
Copy link
Member

Does this fix #9375? cc @mlubin

@mlubin
Copy link
Member

mlubin commented Jan 13, 2016

@simonster, that code isn't used anymore, so it would be a bit of work to dig it up and test

JeffBezanson added a commit that referenced this pull request Jan 13, 2016
use li->roots instead of module constant table
@JeffBezanson JeffBezanson merged commit b0cd2ad into master Jan 13, 2016
@yuyichao yuyichao deleted the jb/constanttable branch January 13, 2016 15:47
@tkelman
Copy link
Contributor

tkelman commented Sep 13, 2016

to backport this to release-0.4 (for #14113), there are some remaining mentions of constant_table here

julia/src/gf.c

Lines 1598 to 1600 in 3b06c86

if (m->constant_table != NULL) {
for(i=0; i < jl_array_len(m->constant_table); i++) {
jl_value_t *el = jl_cellref(m->constant_table,i);
- is that block safe to remove?

@JeffBezanson
Copy link
Sponsor Member Author

It won't affect normal operation, but will lead to --compile=all missing some functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants