Bug 206365 – Separate storage of Structure::m_offset into transition and max offset

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

[Justin Michaud]

Right now, deleteProperty/removePropertyTransition causes a structure transition to uncacheable dictionary. Other transitions assume that the offset is monotonically increasing. In order to support structure transitions for deletion, we separate the offset that was added/deleted from the last offset.

[Justin Michaud]

Created attachment 387937 [details]
Patch

[Tadeu Zagallo]

Comment on attachment 387937 [details]
Patch

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

Overall looks good, just a few comments, but mostly nits.

> Source/JavaScriptCore/runtime/Structure.cpp:388
> +        RELEASE_ASSERT(i == structures.size()-1 || structure->lastOffset() > structures[i+1]->lastOffset());

nit: space around operators. Also, I don't think you need this comment, or you could always add a FIXME.

> Source/JavaScriptCore/runtime/Structure.h:357
> +        if (m_lastOffset == (uint16_t)(-1)) {

A couple comments here:
1) Please use a constant for this.
2) use instead std::numeric_limits<uint16_t>::max() and std::numeric_limits<uint16_t>::max() - 1

> Source/JavaScriptCore/runtime/Structure.h:370
> +        else if (offset < (uint16_t)(-2))

nit: I'd do `if (offset == invalidOffset)` and `if (offset != reservedOffset)` or something like that, instead of using `<` here. This way it's more explicit that you are reserving two values.

[Saam Barati]

Comment on attachment 387937 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        Right now, deleteProperty/removePropertyTransition causes a structure transition to uncacheable dictionary. Other transitions 
> +        assume that the offset is monotonically increasing. In order to support structure transitions for deletion, we separate the offset that was added/deleted from the last offset.
> +
> +        We split the existing m_offset into two 16-bit fields, and put them in rare data if they overflow.

Your changelog should contain much more detail on what you're doing.
(e.g, name of the fields, and what they mean, and why it's needed, etc)

> Source/JavaScriptCore/runtime/Structure.cpp:387
> +        // temporary no deletes

I'm unsure what this means

> Source/JavaScriptCore/runtime/Structure.cpp:390
> +        auto lo = lastOffset();

why is this a local variable?

Also, if it is, let's not abbreviate. You can just do:

auto lastOffset = this->lastOffset();

> Source/JavaScriptCore/runtime/Structure.cpp:778
> +        setLastOffset(vm, offset);

Can we also assert here that m_offsetInPrevious is invalidOffset?

> Source/JavaScriptCore/runtime/Structure.cpp:973
> +            setOffsetInPrevious(vm, newOffset);

this feels hokey. I feel like if we rename this to something like "transitionOffset", this code will be more obviously making less sense. Why would this need to be updated here? We're not transitioning.

> Source/JavaScriptCore/runtime/Structure.h:828
> +    // these do not account for anonymous slots

Is this comment even relevant anymore?

[Mark Lam]

Comment on attachment 387937 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:11
>> +        Right now, deleteProperty/removePropertyTransition causes a structure transition to uncacheable dictionary. Other transitions 
>> +        assume that the offset is monotonically increasing. In order to support structure transitions for deletion, we separate the offset that was added/deleted from the last offset.
>> +
>> +        We split the existing m_offset into two 16-bit fields, and put them in rare data if they overflow.
> 
> Your changelog should contain much more detail on what you're doing.
> (e.g, name of the fields, and what they mean, and why it's needed, etc)

From reading thru the patch, it looks like this patch is only working on splitting up m_offset.  The first time, I read this ChangeLog, I misread it to say that you'll be mean that you're implementing the delete property change right now.

How about expressing the ChangeLog something like this ...
=== BEGIN ===
This patch splits the existing Structure::m_offset field into two 16-bit fields: m_lastOffset and m_offsetInPrevious.  There are also 32-bit versions of these fields in StructureRareData.  If the offset value we need to store exceed MaxShortOffset, we'll store a LongOffset value in the 16-bit field, and store the actual offset in the 32-bit equivalent in StructureRareData.  This change is needed in preparation for later implementing a change to how we delete properties.

... <here, (1) explain the problem with how we delete properties now, (2) explain why you need to split m_offset and how that makes it possible to delete properties without transitioning the structure to an uncacheable dictionary. and (3) what are the meaning of m_lastOffset and m_offsetInPrevious.  This information will help a reviewer understand why you replace m_offset with lastOffset() in some cases, and offsetInPrevious() in others below>.
=== END ===

> Source/JavaScriptCore/runtime/Structure.h:356
> +    PropertyOffset lastOffset() const {

WebKit style says '{' should be positiond on the next line, not at the end of the current line (unless the inline function is a trivial 1 liner).

>> Source/JavaScriptCore/runtime/Structure.h:370
>> +        else if (offset < (uint16_t)(-2))
> 
> nit: I'd do `if (offset == invalidOffset)` and `if (offset != reservedOffset)` or something like that, instead of using `<` here. This way it's more explicit that you are reserving two values.

I think comparing against a max value reads better.  How about something like this:

    static constexpr uint16_t InvalidShortOffset = 0xffff;
    static constexpr uint16_t LongOffset = 0xfffe; // Actual offset value will be stored in StructureRareData.
    static constexpr uint16_t MaxShortOffset = 0xfffd;

Now, setLastOffset() looks like this:

    if (offset <= MaxShortOffset)
        m_lastOffset = offset;
    else if (offset = invalidOffset)
        m_lastOffset = InvalidShortOffset;
    else {
        m_lastOffset = LongOffset;
        ensureRareData(vm)->m_lastOffset = offset;
    }

And lastOffset() can look like:

    if (m_lastOffset <= MaxShortOffset)
        return m_lastOffset;
    if (m_lastOffset == LongOffset)
        return rareData()->m_lastOffset;
    ASSERT(m_lastOffset == InvalidShortOffset);
    return invalidOffset;

I purposely sorted it so that the <= MaxShortOffset case comes first.  Do you have a specific reason for having a RELEASE_ASSERT(hasRareData())?  I think a debug ASSERT should be adequate.  However, you also don't need to add the debug ASSERT because rareData() already ASSERT(hasRareData()).

> Source/JavaScriptCore/runtime/Structure.h:378
> +    PropertyOffset offsetInPrevious() const {

Ditto {

> Source/JavaScriptCore/runtime/Structure.h:388
> +    void setOffsetInPrevious(VM& vm, PropertyOffset offset) {

Ditto {

[Justin Michaud]

Created attachment 387974 [details]
Patch

[Mark Lam]

Comment on attachment 387974 [details]
Patch

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

> Source/JavaScriptCore/runtime/PropertyOffset.h:36
> +static constexpr uint16_t shortInvalidOffset = std::numeric_limits<uint16_t>::max();
> +static constexpr uint16_t shortMaxOffset = std::numeric_limits<uint16_t>::max()-1; // Actual offset value will be stored in StructureRareData.

I think you should keep these "short" values in Structure.h as private static fields for encapsulation reasons.  No one else needs to know about them, right?  They are purely an artifact of Structure's implementation.

> Source/JavaScriptCore/runtime/Structure.h:369
> +        else if (offset < shortMaxOffset && offset < shortInvalidOffset)

Now that you've renamed m_lastOffset to m_maxOffset, shortMaxOffset can sound confusing.  Hmmm, how about renaming it to something like shortUseRareDataOffset (or something that doesn't have "max" in the name?

[Mark Lam]

Comment on attachment 387974 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Separate storage of Structure offset in previous and last offset

Your title still says "previous" and "last".  Please update the names to match the new field names below.

> Source/JavaScriptCore/ChangeLog:21
> +        We split the existing Structure::m_offset into two 16-bit fields, transitionOffset and lastOffset, and put them in 32-bit rare data fields if they overflow. We also rename _inPrevious fields to

Ditto: /lastOffset/maxOffset/ ?

[Justin Michaud]

Created attachment 387988 [details]
Patch

[Saam Barati]

Comment on attachment 387988 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        assume that the transition offset (m_offset) is monotonically increasing. In order to support structure transitions for deletion that 

we should create a bug for that work and link it here for context.

> Source/JavaScriptCore/ChangeLog:10
> +        do not involve turning into a dictionary, we first need to separate the transition offset that was added/deleted from the maximum offset.

you should provide context about what these fields mean, and what they're used for

> Source/JavaScriptCore/runtime/Structure.cpp:387
> +        RELEASE_ASSERT(i == structures.size() - 1 || structure->maxOffset() > structures[i + 1]->maxOffset());

why add this assertion? Won't your next patch immediately make this need to be removed? (E.g, transitions won't necessarily have this in the future).

Also, for perf reasons, might be worth making it a debug assert?

> Source/JavaScriptCore/runtime/Structure.h:393
> +            m_transitionOffset = shortUseRareDataOffset;

why not call this "useRareDataFlag"? or similar. Not a huge fan of "short". I kinda get it for shortInvalidOffset, but feels less needed here

> Source/JavaScriptCore/runtime/Structure.h:828
> +    static constexpr uint16_t shortUseRareDataOffset = std::numeric_limits<uint16_t>::max()-1;

style nit: you need spaces here

[Mark Lam]

Comment on attachment 387988 [details]
Patch

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

r=me too.

>> Source/JavaScriptCore/runtime/Structure.h:393
>> +            m_transitionOffset = shortUseRareDataOffset;
> 
> why not call this "useRareDataFlag"? or similar. Not a huge fan of "short". I kinda get it for shortInvalidOffset, but feels less needed here

I agree.  useRareDataFlag does read better.

[Justin Michaud]

Created attachment 388067 [details]
Patch

[Saam Barati]

(In reply to Mark Lam from comment #10)
> Comment on attachment 387988 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387988&action=review
> 
> r=me too.
> 
> >> Source/JavaScriptCore/runtime/Structure.h:393
> >> +            m_transitionOffset = shortUseRareDataOffset;
> > 
> > why not call this "useRareDataFlag"? or similar. Not a huge fan of "short". I kinda get it for shortInvalidOffset, but feels less needed here
> 
> I agree.  useRareDataFlag does read better.

Or maybe “useRareDataForOffset” flag

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 388067 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 388067 [details]
Patch

Clearing flags on attachment: 388067

Committed r254760: <https://trac.webkit.org/changeset/254760>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58691796>

[Alexey Proskuryakov]

There is a small JSHeap memory regression between r254757-r254767, is it because of this change?

https://perf.webkit.org/v3/#/charts?since=1562116916573&zoom=(1579189862900.3171-1579355018366.603)&paneList=((21-271-37604512)-(21-232-37604482))

I just happened to look at the charts.