846175 - BaselineCompiler: Add optimized stubs for SETPROP adding case.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Found it while analyzing box2d, but this probably shows up in other places too.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Optimized stub for SetProp adding case.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 719601 [details] [diff] [review]
Optimized stub for SetProp adding case.

Review of attachment 719601 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, I also noticed this on v8-richards/raytrace.

::: js/src/ion/BaselineIC.h
@@ +3721,5 @@
> +    }
> +};
> +
> +template <size_t ProtoChainDepth>
> +class ICSetProp_NativeAddImpl : public ICSetProp_NativeAdd

Why is this a separate class? I think it'd be clearer and shorter if we merge it with the NativeAdd class.

Comment 3

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Why is this a separate class? I think it'd be clearer and shorter if we
> merge it with the NativeAdd class.

For a couple of reasons.  We can't call the templated class NativeAdd anyway, since the macro-generated toSetProp_NativeAdd class predeclaration will cause a compile error.

Later, even if we do name it something different, the 'toSetProp_NativeAdd' method will return a pointer to an incomplete type.

With this approach, most users of the toSetProp_NativeAdd class don't have to care about the template specializations (and they shouldn't).  The only reason the template specialization exists is to provide different-sized structs for the allocator.

This approach makes the code a lot cleaner for users of toSetProp_NativeAdd.  I plan to make that even cleaner by adding a method to the base class which lets us access the shape array from the specializations.

With that, client code that deals with the stub pointer will never need to deal with the templated class.  It's a little bit of ugliness in the definition to enable a lot of clarity in the places where this stub gets used.

Comment 4

[Kannan Vijayan [:djvj]]

Pushed: https://hg.mozilla.org/projects/ionmonkey/rev/17df78ddb413

Jan:  If you still feel strongly that the class design for this stub needs to change as you suggested, let me know.

Comment 5

[Kannan Vijayan [:djvj]]

Backed out temporarily to stabilize oranges induced by another patch: https://hg.mozilla.org/projects/ionmonkey/rev/afc621fdea58

Comment 6

[Kannan Vijayan [:djvj]]

Repushed: https://hg.mozilla.org/projects/ionmonkey/rev/de8e40be61d0
Waiting for tbpl before closing.