Cleanup ShapeTable

RESOLVED FIXED in mozilla37

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8545990 [details] [diff] [review]
Part 1 - ShapeTable::search

This patch:

- Moves variables in ShapeTable::search to their first use. This alone makes the algorithm a lot more readable IMO.
- Converts HASH1/HASH2 macros to functions.
- Removes unused SHAPE_IS_LIVE

No change in behavior.
Attachment #8545990 - Flags: review?(n.nethercote)
(Assignee)

Comment 2

3 years ago
Created attachment 8546082 [details] [diff] [review]
Part 2 - Make ShapeTable a proper class

This patch changes ShapeTable from a struct to a class and makes all fields private instead of public.
Attachment #8546082 - Flags: review?(n.nethercote)
Comment on attachment 8545990 [details] [diff] [review]
Part 1 - ShapeTable::search

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

Nice.
Attachment #8545990 - Flags: review?(n.nethercote) → review+
Comment on attachment 8546082 [details] [diff] [review]
Part 2 - Make ShapeTable a proper class

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

::: js/src/vm/Shape.h
@@ +186,5 @@
> +     */
> +    bool            init(ThreadSafeContext *cx, Shape *lastProp);
> +    bool            change(int log2Delta, ThreadSafeContext *cx);
> +
> +    Shape           **search(jsid id, bool adding);

While you're modifying these lines can you remove the excess space between the return type and the function name, above and below? It's ugly and a style that isn't used much. Thanks.
Attachment #8546082 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 5

3 years ago
Created attachment 8546225 [details] [diff] [review]
Part 3 - Add ShapeTable::Entry class

This patch adds a ShapeTable::Entry class that represents an entry in ShapeTable. This is much clearer than the old Shape** (or even Shape*** somewhere) pointers. The SHAPE_* macros like SHAPE_FETCH are now methods of ShapeTable::Entry.

I'm pretty happy this worked out like I hoped it would.

There are a few more things I want to change, for instance we can add bounds checks whenever we get entries_[x]. I'll do that tomorrow.
Attachment #8546225 - Flags: review?(n.nethercote)
(Assignee)

Comment 6

3 years ago
Comment on attachment 8546225 [details] [diff] [review]
Part 3 - Add ShapeTable::Entry class

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

::: js/src/vm/Shape.h
@@ +165,5 @@
> +
> +        void flagCollision() {
> +            shape_ = reinterpret_cast<Shape*>(uintptr_t(shape_) | SHAPE_COLLISION);
> +        }
> +        void storePreservingCollision(Shape *shape) {

I wanted to rename this to setPreservingCollision but forgot, will do that locally.
Comment on attachment 8546225 [details] [diff] [review]
Part 3 - Add ShapeTable::Entry class

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

Bless you.
Attachment #8546225 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8546570 [details] [diff] [review]
Part 4 - Various changes

- hashShift_/log2 values are always in the range [0, 32], so these can be changed from int to uint32_t.

- Adds a getEntry method that asserts the index is in range.

- Adds some other asserts.
Attachment #8546570 - Flags: review?(n.nethercote)
Comment on attachment 8546570 [details] [diff] [review]
Part 4 - Various changes

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

::: js/src/vm/Shape.cpp
@@ +321,5 @@
>  
>      uint32_t size = capacity();
> +    int delta = removedCount_ < (size >> 2);
> +
> +    MOZ_ASSERT(entryCount_ + removedCount_ <= size - 1);

Is the -1 correct? Not sure.
Attachment #8546570 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 10

3 years ago
Parts 1-3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/49f2b5bf30c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/82be31c150f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/61fdafc3b45c
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/49f2b5bf30c5
https://hg.mozilla.org/mozilla-central/rev/82be31c150f5
https://hg.mozilla.org/mozilla-central/rev/61fdafc3b45c
(Assignee)

Comment 12

3 years ago
Part 4:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab841f36d62f

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Is the -1 correct? Not sure.

The next line is:

    if (!change(delta, cx) && entryCount_ + removedCount_ == size - 1) {

So asserting <= size - 1 should be correct.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/ab841f36d62f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.