[webkit-reviews] review granted: [Bug 22377] Add (really) polymorphic caches for prototype property accesses. : [Attachment 25303] The patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 22377: Add (really) polymorphic caches for prototype property accesses.
https://bugs.webkit.org/show_bug.cgi?id=22377

Attachment 25303: The patch
https://bugs.webkit.org/attachment.cgi?id=25303&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Great change! I have some nit-pick-type style comments.

Generally the use of "proto" everywhere to mean "prototype" is a little
confusing to me, because the prefix "proto" already exists and has a different
meaning. I'd prefer that we actually write out prototype unless it makes things
unbearably wordy and long.

> +	   for (int i=0; i < count; ++i) {

Tiny formatting thing: We normally put spaces around the "=" sign.

> +#define ProtoStructureList_LENGTH 4

Does this need to be a macro rather than a const size_t? Similarly, do we need
to use the unconventional capitalization here?

> +	   // Only for the jitterbug, for nows.
> +	   ASSERT_NOT_REACHED();

LOL. I don't really know what the comment means, but it's still funny.

> +    if (baseValue->isObject() &&
> +	   slot.isCacheable() &&
> +	   !asCell(baseValue)->structure()->isDictionary() &&
> +	   slot.slotBase() ==
asCell(baseValue)->structure()->prototypeForLookup(callFrame)) {

We normally try to format these with the operators at the starts of lines
instead of the ends -- I think it makes things read a little bit clearer.

Those two places where you call asCell, I think I'd prefer asObject, because
you've already established it's an object, and it's possible that we might some
day implement an operation like structure() slightly more efficiently for
JSObject than a JSCell. OK, it's not a realistic possibility, but still I like
types to be as specific as possible in case that helps with optimization later.


> +	   JSCell* baseCell = asCell(baseValue);

Same thought here.

I also think that if the values are worth putting into local variables for the
code, maybe it's worth using a nested if and using the local variables in the
if conditional too.

> +	   // Heavy access to a prototype is a good indication that it's not
being
> +	   // used as a dictionary.

Great comment, but I don't understand how the single call here indicates "heavy
use". You might want to word it slightly differently to make that clearer.

> +JSValue* Interpreter::cti_op_get_by_id_proto_list_append(CTI_ARGS)

It's a bit disappointing that so much code is repeated. If there's a way to
factor things out into separate inline functions, that would be nice. Obviously
ARG_xxx causes trouble for that sort of thing, and I wouldn't want to sacrifice
performance, but it's worth considering.

> +    CallFrame* callFrame = ARG_callFrame;
> +    Identifier& ident = *ARG_id2;
> +
> +    JSValue* baseValue = ARG_src1;
> +    PropertySlot slot(baseValue);
> +    JSValue* result = baseValue->get(callFrame, ident, slot);

I personally write these functions using fewer local variables. I would use
ARG_callFrame and *ARG_id2 directly rather than putting them into locals. I'm
not sure why I prefer that style -- I guess I just think that if no clarity is
sacrificed I'd prefer using fewer lines of code and I even think that our ARG
trick might work better if we reduce the chance of confusing the compiler into
allocating another stack location to store the ARG thing again as a local
variable.

It's a minor point, and my habit is not necessarily a good thing. Obviously
naming the things is a form of documentation.

> +    // check eax is an object of the right Structure.

Better to capitalize the first word of a sentence comment like this one.

> +    __ cmpl_i32m(reinterpret_cast<uint32_t>(structure), FIELD_OFFSET(JSCell,
m_structure), X86::eax);

In general, I look for ways to use inline casting functions or function
templates rather than actually sprinkling the code with these reinterpret_cast
and other typecasts. Somehow I think this could make some future refactorings
easier. I think we should make one for casting function pointers to void*, for
example, and use that instead of using reinterpret_cast directly. And the one
for casting pointers to integers. If the functions are suitable task-specific,
then I think they could help improve code readability.

> +    __ cmpl_i32m(reinterpret_cast<uint32_t>(prototypeStructure),
static_cast<void*>(prototypeStructureAddress));

Is the static_cast<void*> needed? Why?

> +    intptr_t successDest = (intptr_t)(stubInfo->hotPathBegin) +
repatchOffsetGetByIdPropertyMapOffset;

Lets use C++ style casts, not C-style, whenever possible.

> +    X86Assembler::link(code, success, reinterpret_cast<void*>(successDest));


We could use char* instead of intptr_t when we need to do pointer arithmetic.
That would eliminate the need for this reinterpret_cast back to void*.

> +    // Track the stub we have created so that it will be deleted later.
> +    //info.stubRoutine = code;

Commented out line of code??? What's up with this?

> +    // FIXME: should revert this repatching, on failure.

Is this something important to get back to?

(Also, please capitalize first word of the sentence comment.)

r=me -- the only thing that worries me is that line of commented-out code