Closed Bug 868042 Opened 11 years ago Closed 11 years ago

Dictionary-mode shapes hurt Octane-gameboy

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Octane-gameboy creates a GameBoyCore object with hundreds of properties (at least 470). This exceeds PropertyTree::MAX_HEIGHT and the object/shape goes into dictionary mode.

If I change PropertyTree::MAX_HEIGHT from 128 to 512 our score improves from 18000 to 31000 points, almost 2x faster.
Re-posting my thoughts from IRC.

If we can devise some heuristics other than number-of-properties that indicate if an object is being used as a struct instead of a hashtable, we can selectively extend MAX_HEIGHT in those situations.

The first thing that came to mind was storing a bit indicating whether or not every property name in ancestor shapes (including the shape containing the bit) have "method-like names" (e.g. matching regex [a-zA-Z_][a-zA-Z0-9]+ or the full regex for JS identifiers to be more complete).

When extending the shape, computing this bit is simple.  If the ancestor is marked as having a non-methodlike property in its chain, then the bit is inherited directly.  Otherwise, the new property name is checked and the bit set based on that.

Looking at gbemu.js, this seems to be the case for GameBoyCore.
Another option is to look for element accesses rather than normal property accesses, similar to how we handle despecializing type information for hashtables.
An issue with looking at the access mode (GETELEM vs GETPROP) is that it would break in case of procedural interface generation.  That tends to be a common idiom in js, including in many popular libraries which provide class-construction helpers where you pass in an object with methods to define and they get copied into the constructor prototype.
Attached patch WIPSplinter Review
Bumping the limit from 128 to 512 regresses unpack-code (3 ms). It looks like it uses an object as hashtable to store identifiers, so there's no good way to prevent this based on the property names.

This patch sets a bit on the shape when SETELEM is used. If the bit is not set, we allow a max height of 512.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Brian asking you to review since you did the ObjShrink work. Let me know if you can think of a better way to do this.
Attachment #746313 - Flags: review?(bhackett1024)
Comment on attachment 746313 [details] [diff] [review]
Patch

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

I think the flag would be better off as a BaseShape::Flag.  These are bits about the object that are automatically preserved by changes made to its shape, so the jspropertytree.cpp change isn't needed.  Doing things this way has a couple problems that hurt robustness:

- Objects that share the initial part of the shape hierarchy with one that is used as a hashmap may still end up with the lower limit, even if they've never seen elements accesses.

- Since the bit is only propagated to children when expanding the property tree, an object which both is used as a hashmap and reuses part of the property tree might not end up with the bit set.

Both of these stem from the fact that the shape is being updated directly, which goes against the general abstraction that shapes are immutable things.  Using the base shape flags for this purpose is how this problem has been handled in the past (e.g. marking objects with uncacheable protos).
Attachment #746313 - Flags: review?(bhackett1024)
Attached patch Patch v2Splinter Review
I wasn't sure about adding the bit to Shape or BaseShape, thanks for the explanation. I followed setUncacheableProto, but I don't know if I should use GENERATE_SHAPE or GENERATE_NONE. Does it matter here?
Attachment #746313 - Attachment is obsolete: true
Attachment #746353 - Flags: review?(bhackett1024)
Comment on attachment 746353 [details] [diff] [review]
Patch v2

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

GENERATE_NONE would be fine, that argument just controls whether an object that is in dictionary mode needs to be reshaped after the change.  Otherwise the last base shape (which is owned by the object) will be modified directly.

::: js/src/jsinterpinlines.h
@@ +911,5 @@
>          }
>      }
>  
> +    if (obj->isNative())
> +        obj->setHadElementsAccess(cx);

The return value needs to be checked here.
Attachment #746353 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/0ea128318cc8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Blocks: 872381
Depends on: 885660
No longer depends on: 885660
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: