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

PERFORMANCE: Intern Multiple String Constants #4754

Closed

Conversation

Projects
None yet
2 participants
@original-brownbear
Copy link
Contributor

original-brownbear commented Aug 27, 2017

JRuby interns most Strings that come up in Ruby code just fine in org.jruby.ir.persistence.IRReaderStream#decodeString:

    public String decodeString() {
        int strLength = decodeInt();

        if (strLength == NULL_STRING) return null;

        byte[] bytes = new byte[strLength]; // FIXME: This seems really innefficient
        buf.get(bytes);

        String newString = new String(bytes).intern();

        if (RubyInstanceConfig.IR_READING_DEBUG) System.out.println("STR<" + newString + ">");

        return newString;
    }

, but a few duplicates from more dynamic contexts are not caught. In the red-black benchmark, this adds up to ~200kb of wasted memory (memory wise this is irrelevant obviously, but duplicates make call site cache lookups slower and waste CPU cache => fixing all these spots resulted in a visible ~2% speedup in the red-black benchmark for me over very large sample sizes 2k+ runs).

So I:

  • Tracked down spots generating duplicate constants for ASTish code paths
  • Did my best to not introduce duplicate calls to intern by interning in constructors wherever possible instead of further upstream
  • Made sure not intern calls made it into the hot path by checking that the method isn't called in the hot path (checked red-black benchmark and Logstash after warm up and intern was never called in either => I have a bit of confidence)

Also:

  • Removed public synchronized void defineAliases(List<String> aliases, String oldName) since it was one of the cases that looked like they would create duplicate String constants but then realized that it's never used (checked for dynamic invocations too)
  • Reduced visibility of public static JavaCallable getMatchingCallable(Ruby runtime, Class<?> javaClass, String methodName, Class<?>[] argumentTypes) since this method was in a path affected by the added intern calls and only used privately => I didn't want to add redundant calls to intern to it, when I can be sure that its only caller interns Strings just fine already
@original-brownbear

This comment has been minimized.

Copy link
Contributor Author

original-brownbear commented Aug 27, 2017

this is not ready, closing until fixed ...

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Aug 28, 2017

@original-brownbear also check out symbol_love which is a branch which will be eliminating any String's from existing. Everything will be RubySymbol and we will have backwards compat signatures for people interacting via Java. That branch is not a sure thing for the future but j.l.String needs to go since it limits our compatibility with non-Java-charset encodings.

@original-brownbear

This comment has been minimized.

Copy link
Contributor Author

original-brownbear commented Aug 28, 2017

@enebo thanks will take a look :)

@enebo enebo modified the milestone: JRuby 9.2.0.0 Sep 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.