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

respect jit.max to stop compilation #5870

Merged
merged 17 commits into from Sep 17, 2019
Merged

respect jit.max to stop compilation #5870

merged 17 commits into from Sep 17, 2019

Conversation

@kares
Copy link
Member

@kares kares commented Sep 10, 2019

... after some time esp. for big (Rails) apps that seemingly always execute some 'new-hot' methods

JIT parts also seemed ripe for a cleanup, so there's a few bits such as :

  • not having multiple callCount in a hierarchy
  • negated JIT condition - JITing when JIT meant to be disabled
  • get JIT excludes to also exclude block compilations
  • more useful ji logging - seeing exception.toString failures

aside from these, there's 2 simple but important changes :

  • -Xjit.max=10000 default, to compilations and not leak meta-space
    even for a large Rails app this should be conservative as it takes several hours to reach the limit
    from a production app 10_000 JIT compiles means having around 550 of allocated meta-space

  • Xjit.maxsize reduced from 2000 -> 1000 again based on real world production data
    there's very little JIT eligible large methods in Rails, so this further helps a bit saving on meta-space

also, since I will like to add JMX setters for the jit params, they are being read from the instance config

Copy link
Member

@headius headius left a comment

Seems like some good cleanup but I had a few questions.

core/src/main/java/org/jruby/compiler/BlockJITTask.java Outdated Show resolved Hide resolved
protected int callCount = 0;
private MethodData methodData;
protected volatile int callCount = 0;
private transient MethodData methodData;
Copy link
Member

@headius headius Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the volatile here for disabling? Since our count is always checking > threshold I never bothered to make this volatile since someone will eventually cross the line. If it's volatile it will force a volatile read for every call.

And why transient?

Copy link
Member Author

@kares kares Sep 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transient just a marker that its a cached field.
method pbly can't serialize state due the IR scope but if it did this field is not needed


runtime.getJITCompiler().buildThresholdReached(context, this);
}
}
Copy link
Member

@headius headius Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is very similar to that inside the method/block jit stuff, isn't it?

Copy link
Member Author

@kares kares Sep 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yy - would need a common super type if we're to keep field read write (this.callCount++)

the setter is synchronized - wanted to avoid sync cost, but it likely does not need to be if it isnt called externally.

@headius
Copy link
Member

@headius headius commented Sep 11, 2019

default JIT max seems reasonable. I wonder if we should warn in some mode (maybe always, maybe only verbose mode, I don't know) that the limit has been reached? 10k methods is a lot, especially if a large Rails app is taking hours to get there.

2000 to 1000 limit seems reasonable because this is IR count and IR is much more dense than bytecode. I'll be doing some review of all IR jit logic to see what else we can shrink, also.

Live JIT flags seem good too...useful for tuning exploration etc.

kares added 10 commits Sep 12, 2019
remove duplicate code and share JIT-ing attempt between method impls

since we do not mind delaying JIT let's not sync callCount r/w
the counter will eventually reach the threshold (from a single thread)
still make sure we do not submit a method twice by setting, and always
checking, a low (negative) call count value
... using the same "fast" call counter algorithm
previously compilation was unbounded, JRuby will know use a threshold
at which compiling stops - potentially to avoid leaking meta-space
that are within/belong to specified exclude targets
in a typical Rails app there are only a few JIT eligible methods
of > 1000 instructions, thus a higher default does not make sense

on a production Rails app a lower default reduced the overall generated
code size by 10% wout negative performance degradation,
while avg code size went from 8049 -> 7860 bytes
its fine for us to loose some counts from other threads
- we always check for >= 0
- setting to a negative value while counting now uses MIN_VALUE
  and is synchronized
(in interpreted method/body) to disable build promotion
full builds are also promoted when compilation is OFF
@kares
Copy link
Member Author

@kares kares commented Sep 12, 2019

logging for eligible JIT methods being skipped due max reached is there (did not want to stop the queue, the next thing I want to do is to allow changing JIT related config from JMX): with -Xjit.logging
https://github.com/jruby/jruby/pull/5870/files#diff-0da2daefadcea7c708815b454db3d439R184

kares added 6 commits Sep 13, 2019
so we can re-instantiate byte-code size check in a single place
since we'd like to know the total number of compiled classes
implementation class will now return enclosing scope module,
with updated log messages we already distinguish block/method
@kares
Copy link
Member Author

@kares kares commented Sep 14, 2019

now updated with more work, parts of which I am going to need to test out JITing in production.
(namely the possibility to change jit parameters from JMX - code should be fine to handle that)

also, have tested locally - block JIT excludes work as well as reaching jit.max halts compilation

@headius
Copy link
Member

@headius headius commented Sep 16, 2019

I'm fine merging this any time. @enebo should probably coordinate with you because I think he was playing with some stuff that might overlap.

@kares
Copy link
Member Author

@kares kares commented Sep 16, 2019

actually do have @enebo's counter_nuke work merged on top of this (in another branch) with some fine tuning (ability to disable jit.time.delta)

@enebo
Copy link
Member

@enebo enebo commented Sep 16, 2019

@kares @headius I think merging this will be fine even it counter_nuke ends up solving the original issue as well. if aging works really well then a jit.max may never be used but I could see it as an insurance policy.

A second thought is we should consider decrementing jit count if an oneshotclassloader unloads because the method went away. I do not think that is important for this PR and that may be more complicated with the chunked classloading I am looking at. Still something to think about.

@kares
Copy link
Member Author

@kares kares commented Sep 17, 2019

good, was also thinking of having an insurance policy (jit.max) regardless of other work ...
merging, will put up further work for review (on top of this) - the class-loader sharing piece.

@kares kares merged commit 7103c22 into jruby:master Sep 17, 2019
4 of 5 checks passed
@kares kares added this to the JRuby 9.2.9.0 milestone Sep 17, 2019
@kares kares added the jit label Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants