Bug 125172 – ObjectAllocationProfile is racy and the DFG should be cool with that

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming

<rdar://problem/15233487>

[Filip Pizlo]

Created attachment 218318 [details]
the patch

[Mark Hahnenberg]

Comment on attachment 218318 [details]
the patch

r=me

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/160038

[Darin Adler]

Comment on attachment 218318 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218318&action=review

> Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:156
> +    ASSERT(structure);

When you find yourself writing this assertion, it’s worth considering making the argument Structure& instead of Structure*.

> Source/JavaScriptCore/runtime/JSFunction.h:141
> +        Structure* allocationStructure() { return m_allocationProfile.structure(); }

If this can never be null we should consider returning Structure&.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 218318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218318&action=review
> 
> > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:156
> > +    ASSERT(structure);
> 
> When you find yourself writing this assertion, it’s worth considering making the argument Structure& instead of Structure*.

This would be strange.  We use * for JSC heap pointers and Structure is a JSC heap object.  There is no precedent for using & to refer to JSC heap objects.  I expect accesses to the JSC heap to use -> and not ".".

It would be weird to me if the same kind of object was referred to using both & and *.  This is confusing; you end up with a mix of . and -> to access its members, and you find yourself having to use & and * in a bunch of places to convert from one pointer style to the other.  We have this issue with JSC::VM and it's really gross.

But overall, I just really don't like &.  I wish it wasn't part of the language.

> 
> > Source/JavaScriptCore/runtime/JSFunction.h:141
> > +        Structure* allocationStructure() { return m_allocationProfile.structure(); }
> 
> If this can never be null we should consider returning Structure&.

It can be null, and that's OK.

[Darin Adler]

(In reply to comment #5)
> But overall, I just really don't like &.  I wish it wasn't part of the language.

OK, that seems like the real issue.

We need to get you together with the folks currently setting the style elsewhere in WebKit, because elsewhere we are seeing improvements that come from switching from pointers to references.

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #5)
> > But overall, I just really don't like &.  I wish it wasn't part of the language.
> 
> OK, that seems like the real issue.
> 
> We need to get you together with the folks currently setting the style elsewhere in WebKit, because elsewhere we are seeing improvements that come from switching from pointers to references.

I think that the bigger issue for this particular case is just that we always use pointers, not references, to refer to things allocated in the JS heap (i.e. JSCell and friends).  We then leverage this with idioms that either allow or require JSCell* to possibly be null, for example:

if (JSObject* object = jsDynamicCast<JSObject*>(jsValue)) {
    // do things for the case that jsValue is an object.
} else {
    // do things for the case that it ain't
}

This is sort of fitting because of the dynamic nature of JSCells and JSValues.  They are the C++-side proxies for values in a dynamic language, and so it is quite rare that we have a guarantee like "I know that I have a pointer to Blah and it's not null".

There may be a separate debate regarding whether & is better than * for WebCore or broadly for "most objects" in WebKit.  Even if the outcome of that debate is (or was, past tense) that & is usually better, I still think it's sensible to ask questions like:

- Does it make the code better, or worse, if we make one particular reference to an object use & even though it's a convention to always use * for that object?

- Is it a good use of time to attempt to change all or some of the * pointers to a kind of object into & references?

- If we incrementally start changing all references to an object to be & instead of *, how much of a cost will this incur in the meantime for other people trying to do things to the codebase?

For JSCell, I propose that the answers are, respectively: worse, bad use, too much.  The reason for my pessimism is that I would guess that there will be very few places where we can reasonably use & instead of * for JSCells and so those few places will just stick out like sore thumbs.  For example, they will degrade readability because those of us who stare at the code the most are accustomed to "." and "&" meaning "it ain't GC'd".  Getting to the point where we find those places (where JSCell& would work at all) will take a non-trivial amount of effort and will produce relatively small gains.

The trade-off may be different for other kinds of objects.  Personally, I would always err on the side of * instead of &, but in my arguments above I'm trying to step back from that bias and make a more objective argument.