Skip to content

Commit

Permalink
Showing 1 changed file with 18 additions and 6 deletions.
Original file line number Diff line number Diff line change
@@ -1188,20 +1188,23 @@ public DynamicObject stringByteAppend(DynamicObject string, DynamicObject other)
@ImportStatic(StringGuards.class)
public static abstract class StringSubstringPrimitiveNode extends RubiniusPrimitiveNode {

@Child private AllocateObjectNode allocateNode;
@Child private TaintResultNode taintResultNode;
private final ConditionProfile negativeLengthProfile = ConditionProfile.createBinaryProfile();
private final ConditionProfile tooLargeBeginProfile = ConditionProfile.createBinaryProfile();
private final ConditionProfile negativeBeginProfile = ConditionProfile.createBinaryProfile();

This comment has been minimized.

Copy link
@eregon

eregon Oct 14, 2015

Member

Profiles should in general not be defined in the base node itself, as there only one instance of that one for N specializations. In this case it's fine because they are only used in one specialization but if they were in multiple specializations it would cause profile pollution between specializations.
A way to have per-specialization profiles as it is supposed to be is with @Cached, as in AppendOneNode for example. It also avoid allocating the profile if not used.
@chrisseaton Am I correct? Should we try to unify the style for profiles?

This comment has been minimized.

Copy link
@chrisseaton

chrisseaton Oct 14, 2015

Contributor

Yeah we already have the guidance you wrote in the wiki which make sense to me. Things like condition profiles are especially easy to do in the DSL.

This comment has been minimized.

Copy link
@nirvdrum

nirvdrum Oct 14, 2015

Author Contributor

Sorry, I forgot about the sharing (and the wiki). I'll update accordingly.


public StringSubstringPrimitiveNode(RubyContext context, SourceSection sourceSection) {
super(context, sourceSection);
}

public abstract Object execute(VirtualFrame frame, DynamicObject string, int beg, int len);

@TruffleBoundary
@Specialization(guards = "isSingleByteOptimizable(string)")
public Object stringSubstringSingleByteOptimizable(DynamicObject string, int beg, int len) {
// Taken from org.jruby.RubyString#substr19.

if (len < 0) {
if (negativeLengthProfile.profile(len < 0)) {
return nil();
}

@@ -1210,14 +1213,14 @@ public Object stringSubstringSingleByteOptimizable(DynamicObject string, int beg
len = 0;
}

if (beg > length) {
if (tooLargeBeginProfile.profile(beg > length)) {
return nil();
}

if (beg < 0) {
beg += length;

if (beg < 0) {
if (negativeBeginProfile.profile(beg < 0)) {
return nil();
}
}
@@ -1310,13 +1313,22 @@ public Object stringSubstring(DynamicObject string, int beg, int len) {
private DynamicObject makeSubstring(DynamicObject string, int beg, int len) {
assert RubyGuards.isRubyString(string);

if (allocateNode == null) {
CompilerDirectives.transferToInterpreter();
allocateNode = insert(AllocateObjectNodeGen.create(getContext(), getSourceSection(), null, null));
}

if (taintResultNode == null) {
CompilerDirectives.transferToInterpreter();
taintResultNode = insert(new TaintResultNode(getContext(), getSourceSection()));
}

final DynamicObject ret = Layouts.STRING.createString(Layouts.CLASS.getInstanceFactory(Layouts.BASIC_OBJECT.getLogicalClass(string)), new ByteList(StringOperations.getByteList(string), beg, len), StringSupport.CR_UNKNOWN, null);
StringOperations.getByteList(ret).setEncoding(StringOperations.getByteList(string).getEncoding());
final DynamicObject ret = allocateNode.allocate(
Layouts.BASIC_OBJECT.getLogicalClass(string),
new ByteList(StringOperations.getByteList(string).unsafeBytes(), beg, len),
StringSupport.CR_UNKNOWN,
null);

taintResultNode.maybeTaint(string, ret);

return ret;

0 comments on commit 09ecd35

Please sign in to comment.