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.

Sign up for GitHub

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

Jump to bottom

MINOR: Make CachingCallSite more JIT friendly #4753

Merged
merged 1 commit into from Aug 29, 2017
Merged

MINOR: Make CachingCallSite more JIT friendly #4753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 29 additions & 12 deletions core/src/main/java/org/jruby/RubyModule.java
View file
Open in desktop
Expand Up @@ -38,8 +38,6 @@
package org.jruby;

import com.headius.invokebinder.Binder;
import org.jcodings.Encoding;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.reflect.Field;
Expand All @@ -58,7 +56,7 @@
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;

import org.jcodings.Encoding;
import org.jruby.anno.AnnotationBinder;
import org.jruby.anno.AnnotationHelper;
import org.jruby.anno.JRubyClass;
Expand Down Expand Up @@ -118,9 +116,21 @@
import org.jruby.util.log.Logger;
import org.jruby.util.log.LoggerFactory;

import static org.jruby.anno.FrameField.*;
import static org.jruby.anno.FrameField.BACKREF;
import static org.jruby.anno.FrameField.BLOCK;
import static org.jruby.anno.FrameField.CLASS;
import static org.jruby.anno.FrameField.FILENAME;
import static org.jruby.anno.FrameField.JUMPTARGET;
import static org.jruby.anno.FrameField.LASTLINE;
import static org.jruby.anno.FrameField.LINE;
import static org.jruby.anno.FrameField.METHODNAME;
import static org.jruby.anno.FrameField.SCOPE;
import static org.jruby.anno.FrameField.SELF;
import static org.jruby.anno.FrameField.VISIBILITY;
import static org.jruby.runtime.Visibility.*;
import static org.jruby.runtime.Visibility.MODULE_FUNCTION;
import static org.jruby.runtime.Visibility.PRIVATE;
import static org.jruby.runtime.Visibility.PROTECTED;
import static org.jruby.runtime.Visibility.PUBLIC;


/**
Expand Down Expand Up @@ -1311,20 +1321,27 @@ public DynamicMethod searchWithRefinements(String name, StaticScope refinedScope
* @param cacheUndef Flag for caching UndefinedMethod. This should normally be true.
* @return The method, or UndefinedMethod if not found
*/
public CacheEntry searchWithCache(String name, boolean cacheUndef) {
CacheEntry entry = cacheHit(name);

if (entry != null) return entry;
public final CacheEntry searchWithCache(String name, boolean cacheUndef) {
final CacheEntry entry = cacheHit(name);
return entry != null ? entry : searchWithCacheMiss(name, cacheUndef);
}

/**
* Search through this module and supermodules for method definitions after {@link RubyModule#cacheHit(String)}
* failed to return a result. Cache superclass definitions in this class.
*
* @param name The name of the method to search for
* @param cacheUndef Flag for caching UndefinedMethod. This should normally be true.
* @return The method, or UndefinedMethod if not found
*/
private CacheEntry searchWithCacheMiss(final String name, final boolean cacheUndef) {
// we grab serial number first; the worst that will happen is we cache a later
// update with an earlier serial number, which would just flush anyway
int token = getGeneration();
final int token = generation;
DynamicMethod method = searchMethodInner(name);

if (method instanceof CacheableMethod) {
method = ((CacheableMethod) method).getMethodForCaching();
}

return method != null ? addToCache(name, method, token) : cacheUndef ? addToCache(name, UndefinedMethod.getInstance(), token) : cacheEntryFactory.newCacheEntry(name, method, token);
}

Expand Down
22 changes: 11 additions & 11 deletions core/src/main/java/org/jruby/ast/executable/RuntimeCache.java
View file
Open in desktop
Expand Up @@ -405,7 +405,7 @@ public IRubyObject reCacheFrom(RubyModule target, ThreadContext context, String

public DynamicMethod getMethod(ThreadContext context, RubyClass selfType, int index, String methodName) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, selfType)) {
if (myCache.typeOk(selfType)) {
return myCache.method;
}
return cacheAndGet(context, selfType, index, methodName);
Expand Down Expand Up @@ -448,7 +448,7 @@ private DynamicMethod searchWithCacheNoMethodMissing(RubyClass clazz, int index,
// used from byte-code generated by RealClassGenerator
public final DynamicMethod searchWithCacheNoMethodMissing(IRubyObject obj, int index, String name1) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCacheNoMethodMissing(obj.getMetaClass(), index, name1);
Expand All @@ -457,7 +457,7 @@ public final DynamicMethod searchWithCacheNoMethodMissing(IRubyObject obj, int i
// used from byte-code generated by RealClassGenerator
public final DynamicMethod searchWithCacheNoMethodMissing(IRubyObject obj, int index, String name1, String name2) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCacheNoMethodMissing(obj.getMetaClass(), index, name1, name2);
Expand Down Expand Up @@ -545,63 +545,63 @@ public DynamicMethod searchWithCache(RubyClass clazz, int index, String name1, S

public DynamicMethod searchWithCache(IRubyObject obj, int index, String name1) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCache(obj.getMetaClass(), index, name1);
}

public DynamicMethod searchWithCache(IRubyObject obj, int index, String name1, String name2) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCache(obj.getMetaClass(), index, name1, name2);
}

public DynamicMethod searchWithCache(IRubyObject obj, int index, String name1, String name2, String name3) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCache(obj.getMetaClass(), index, name1, name2, name3);
}

public DynamicMethod searchWithCache(IRubyObject obj, int index, String name1, String name2, String name3, String name4) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCache(obj.getMetaClass(), index, name1, name2, name3, name4);
}

public DynamicMethod searchWithCache(IRubyObject obj, int index, String name1, String name2, String name3, String name4, String name5) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCache(obj.getMetaClass(), index, name1, name2, name3, name4, name5);
}

public DynamicMethod searchWithCache(IRubyObject obj, int index, String name1, String name2, String name3, String name4, String name5, String name6) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCache(obj.getMetaClass(), index, name1, name2, name3, name4, name5, name6);
}

public DynamicMethod searchWithCache(IRubyObject obj, int index, String name1, String name2, String name3, String name4, String name5, String name6, String name7) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCache(obj.getMetaClass(), index, name1, name2, name3, name4, name5, name6, name7);
}

public DynamicMethod searchWithCache(IRubyObject obj, int index, String name1, String name2, String name3, String name4, String name5, String name6, String name7, String name8) {
CacheEntry myCache = getCacheEntry(index);
if (CacheEntry.typeOk(myCache, obj.getMetaClass())) {
if (myCache.typeOk(obj.getMetaClass())) {
return myCache.method;
}
return searchWithCache(obj.getMetaClass(), index, name1, name2, name3, name4, name5, name6, name7, name8);
Expand Down
6 changes: 1 addition & 5 deletions core/src/main/java/org/jruby/runtime/callsite/CacheEntry.java
View file
Open in desktop
Expand Up @@ -15,11 +15,7 @@ public CacheEntry(DynamicMethod method, int token) {
}

public final boolean typeOk(RubyClass incomingType) {
return typeOk(this, incomingType);
}

public static final boolean typeOk(CacheEntry entry, RubyClass incomingType) {
Copy link
Member

Choose a reason for hiding this comment

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

you need to careful when removing methods on places such as these.
might be called from JIT-ed byte-code (so you're not seeing it show up as used)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kares yea, I learnt that today in some other spot already :) Questions (if you have a sec):

  1. Can I do anything besides running tests + full-text search to make sure I don't kill such a method accidentally?
  2. Am I correct to understand that the @JIT annotation should be put on methods which could be called dynamically like you described?

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Can I do anything besides running tests + full-text search to make sure I don't kill such a method accidentally?

usually just grep for the method-name string -> "typeOf" should be safe to find whether its used :)

Am I correct to understand that the @jit annotation should be put on methods which could be called dynamically like you described?

well yeah - have run into that as well ... lately.
but what I dislike is that now some places might have a dependency on the IR package (org.jruby.ir.JIT) -> should maybe get relocated somewhere else I guess ... not sure
haven't raised that concern yet -> you should chat with @enebo or @headius on IRC if it was meant to be somehow IR specific or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kares thanks!

return entry.token == incomingType.getGeneration();
return token == incomingType.getGeneration();
}

@Override
Expand Down