Skip to content

Commit

Permalink
[Truffle] Cache of integers formatted as ropes.
Browse files Browse the repository at this point in the history
@chrisseaton
chrisseaton committed Apr 10, 2016
1 parent 7e7f844 commit 7d82684
Showing 2 changed files with 30 additions and 2 deletions.
4 changes: 2 additions & 2 deletions truffle/src/main/java/org/jruby/truffle/core/numeric/FixnumNodes.java
View file
Original file line number Diff line number Diff line change
@@ -27,6 +27,7 @@
import org.jruby.truffle.core.CoreMethod;
import org.jruby.truffle.core.CoreMethodArrayArgumentsNode;
import org.jruby.truffle.core.Layouts;
import org.jruby.truffle.core.rope.RopeConstants;
import org.jruby.truffle.language.NotProvided;
import org.jruby.truffle.language.RubyNode;
import org.jruby.truffle.language.control.RaiseException;
@@ -1121,10 +1122,9 @@ public ToSNode(RubyContext context, SourceSection sourceSection) {
super(context, sourceSection);
}

@TruffleBoundary
@Specialization
public DynamicObject toS(int n, NotProvided base) {
return create7BitString(Integer.toString(n), USASCIIEncoding.INSTANCE);
return createString(RopeConstants.getIntegerRope(n));
}

@TruffleBoundary
28 changes: 28 additions & 0 deletions truffle/src/main/java/org/jruby/truffle/core/rope/RopeConstants.java
View file
Original file line number Diff line number Diff line change
@@ -10,10 +10,16 @@

package org.jruby.truffle.core.rope;

import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
import org.jcodings.specific.ASCIIEncoding;
import org.jcodings.specific.USASCIIEncoding;
import org.jcodings.specific.UTF8Encoding;

import java.lang.ref.WeakReference;
import java.nio.charset.StandardCharsets;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

public class RopeConstants {

public static final LeafRope EMPTY_ASCII_8BIT_ROPE;
@@ -48,4 +54,26 @@ public class RopeConstants {
}
}

private static final Map<Integer, WeakReference<LeafRope>> integerRopes = new ConcurrentHashMap<>();

@TruffleBoundary
public static LeafRope getIntegerRope(int value) {
WeakReference<LeafRope> ropeReference = integerRopes.get(value);

if (ropeReference != null && ropeReference.get() != null) {
return ropeReference.get();
}

// On misses we don't care too much about racing to populate the cache

final LeafRope rope = new AsciiOnlyLeafRope(
Integer.toString(value).getBytes(StandardCharsets.UTF_8), USASCIIEncoding.INSTANCE);

ropeReference = new WeakReference<>(rope);

integerRopes.put(value, ropeReference);

return rope;
}

}

8 comments on commit 7d82684

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nirvdrum seems like a good idea to cache this.

@thedarkone
Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisseaton I think this will lead to trouble: yes, GC will collect LeapRope string objs from the weak reference, but the key-value mapping (and its associated Map.Entry) will stick around, leaking memory.

Are you trying to tackle the issue of to_s Strings always escaping (due to EA non being able to handle var-length objs)? If so, it might be worth it to profile max length of strs and allocate fixed-width Ropes, the same way you do it for arrays? DynamicObj + Rope (+ byte[] in Rope) is quite heavy already, so an overhead of a few unused bytes might be worth EA-ability?

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I suppose it's quite likely someone could run in a loop formatting random integers for a long time.

The EA can handle variable length objects though - all our argument arrays are EAd. I was just trying to cache what I thought would be common objects. At the moment going from Integer to Rope I think means Integer -> byte[] -> char[] -> byte[], so it's not ideal to be happening a lot, especially in cases like backtraces where they're probably never even seen most of the time.

Maybe what I really wanted was a FormatIntegerRope. Leave the numbers in there lazily. If they're just created and then printed to a file stream or something we can delay the formatting a long way.

@thedarkone
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I suppose it's quite likely someone could run in a loop formatting random integers for a long time.

@headius @enebo from your experience, isn't it the case, that this something that is almost certainly guaranteed to happen?

The EA can handle variable length objects though - all our argument arrays are EAd.

This is probably because during PE/compilation, the args array looks fixed-length to Graal. AFAIK hotpost requires compiled methods to have fixed stack size, so var-length EA is not feasible right now.

Back to my original suggestion. If the return value of Fixnum#to_s doesn't escape, EA will remove DynObj + Rope (+ Integer.toString's String obj) allocations, leaving behind only a tiny char[], so my fixed-width profile-driven idea is almost certainly not worth it.

Sorry, something went wrong.

@thedarkone
Copy link
Contributor

Choose a reason for hiding this comment

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

After some thought, consider e.g. Rails's ORM: there will be a ton of Product.find(params[:id]) calls, where params[:id] is a HTTP provided string, that will be parsed into a Fixnum (for security purposes) and then subsequently internally used to generate a SQL query: "SELECT * FROM products WHERE id = #{id}" ...

PS: sorry for spamming everybody.

Sorry, something went wrong.

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think I'll try the lazy integer node method. I won't leave this cache in place as in as it is not limited by anything.

@eregon
Copy link
Member

@eregon eregon commented on 7d82684 Apr 11, 2016

Choose a reason for hiding this comment

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

One could also locally cache with @Cached in Fixnum#to_s. It would probably be interesting to rewrite Integer.toString ourselves to be more PE-friendly. If it turns out multiple integers output a String of the same length, it might allow interesting optimizations further.

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're flying blind here - I'll produce some benchmarks to show what I'm trying to do.

Please sign in to comment.