[Webkit-unassigned] [Bug 67993] Prediction tracking is not precise enough

Tue Sep 13 18:13:36 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=67993

--- Comment #6 from Filip Pizlo <fpizlo at apple.com>  2011-09-13 18:13:36 PST ---
(In reply to comment #5)
> (From update of attachment 107236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107236&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:813
> > +		0FD82E82141F3FC900179C94 /* BoundsCheckedPointer.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = BoundsCheckedPointer.h; sourceTree = "<group>"; };
> > +		0FD82E84141F3FDA00179C94 /* PredictedType.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = PredictedType.cpp; sourceTree = "<group>"; };
> 
> You'll need to add these guys to the other project files (JavaScriptCore.vcproj, etc.).

Will do!

> 
> > Source/JavaScriptCore/bytecode/PredictedType.h:42
> > +static const PredictedType PredictObjectOther   = 0x0010; // It's definitely an object but not JSFinalObject or JSArray.
> > +static const PredictedType PredictSomeObject    = 0x0020; // It's some unknown subclass of JSObject.
> > +static const PredictedType PredictObject        = 0x003f; // It's definitely a subclass of JSObject.
> 
> The distinction between these three object prediction terms is super confusing to me. Can you clarify somehow?

PredictObjectOther = I definitely saw something that was neither JSFinalObject nor JSArray, but whatever it was, it was definitely a subclass of JSObject.

PredictSomeObject = I saw some objects but I was too lazy to figure out what kinds of objects they were.

PredictObject = This is not meant to be a value that we set PredictedType's to; it's a bit pattern we use to test if it would be OK to speculate that a value is a subtype of JSObject.  If we did set a variable to PredictObject, it would mean that we know for sure that this variable will point to every kind of object.

The interesting thing is what happens when you merge them:

PredictSomeObject + PredictFinalObject = PredictFinalObject.  PredictSomeObject means "I was too lazy to look beyond it being an object", so PredictFinalObject prevails.  Similarly for PredictSomeObject + <any other object prediction>.

PredictFinalObject + PredictOtherObject = PredictFinalObject|PredictOtherObject.  Note that this is not equal to PredictObject, but it is interpreted in a similar way.  It means: we know it's an object but we know that it would be unwise to speculate what kind of object it is.  But in this case the bitpattern still tells us something interesting: it tells us that we never saw arrays.

PredictFinalObject + PredictObject = PredictObject.  PredictObject means somebody definitely saw all kinds of objects, while PredictFinalObject means someone just saw JSFinalObjects.  So we take the union of the two, which is PredictObject.

At a thousand foot view, PredictSomeObject and PredictObject sort of mean the same thing: they both mean that it's safe to assume, based on current profiling information, that the value is an object.  The difference is that:

1) If one guy says PredictSomeObject and another guy gives a different object prediction, then the latter guy wins.  If one guy says PredictObject, then he always trumps everything else.

2) We never explicitly set PredictObject as a prediction.  We only use it as a bitmask to see if we saw any kinds of objects.  The only way that a value prediction would become exactly PredictObject is if we have evidence that suggests that we saw all of the following: JSFinalObject, JSArray, and some object that is neither JSFinalObject nor JSArray.

PredictOtherObject is used to distinguish seeing "some kind of object" (which is what PredictObject and PredictSomeObject do) and seeing an object that we definitely can't optimize for.  We can optimize for JSFinalObject (because it's easy to check that an object is JSFinalObject, so it's a shortcut for speculating on object).  We can, and do, optimize for JSArray.  But we don't want to emit JSFinalObject speculation if we know for sure that the value may be, for instance, JSDateInstance.  Since we don't want to have a bit for every possible subclass of JSObject in PredictedType, we use PredictOtherObject as a placeholder.

Note that it would be unwise to replace PredictOtherObject with PredictObject, since PredictObject means that we've definitely seen JSFinalObject and JSArray.  If PredictOtherObject was just set to PredictObject then we wouldn't be able to detect if an object is never a JSFinalObject, since anytime the value profiler notices, say, a JSFunction, then it would set the bit that corresponds to JSFinalObject (since PredictObject has all object bits set).

> 
> > Source/JavaScriptCore/bytecode/PredictedType.h:60
> >  enum PredictionSource { WeakPrediction, StrongPrediction };
> >  
> >  inline bool isCellPrediction(PredictedType value)
> >  {
> > -    return (value & PredictCell) == PredictCell && !(value & ~(PredictArray | PredictionTagMask));
> > +    return !!(value & PredictCell) && !(value & ~(PredictCell | PredictionTagMask));
> > +}
> 
> Seems like this could all become an object with member functions, wrapping a uint16_t. Not necessary in this patch, though.

Yeah, we're definitely heading in that direction!

> 
> > Source/JavaScriptCore/wtf/BoundsCheckedPointer.h:7
> > + *  This library is free software; you can redistribute it and/or
> > + *  modify it under the terms of the GNU Library General Public
> > + *  License as published by the Free Software Foundation; either
> > + *  version 2 of the License, or (at your option) any later version.
> 
> Is this newly authored code or LGPL2 code from somewhere else? If newly authored, BSD license, please.

New code.  I copy pasted the wrong license header, I guess!  I'll pull in a BSD license header.

> 
> > Source/JavaScriptCore/wtf/BoundsCheckedPointer.h:38
> > +class BoundsCheckedPointer {
> 
> Probably overkill to do in this patch, but it seems like this class could become an iterator for FixedArray<typename, size_t>. That would simplify the use case a bit, and guarantee that the data and the bounds checked pointer were in agreement about the data's size.

Yup!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.