Bug 156123 – Migrate BidiRunList and BidiRun to automatic memory management

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Migrate BidiRunList and BidiRun to automatic memory management

[Myles C. Maxfield]

Created attachment 275451 [details]
Patch

[Myles C. Maxfield]

This is fixing a problem that has been around since r3627 (February 2003)!!!

[Myles C. Maxfield]

Created attachment 275452 [details]
Patch

[Myles C. Maxfield]

Created attachment 275453 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 275453 [details]
Patch

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

> Source/WebCore/platform/text/BidiResolver.h:160
> +        , m_override(context->override())

Why isn't m_level initialized here?

> Source/WebCore/platform/text/BidiResolver.h:197
> -    // Do not add anything apart from bitfields until after m_next. See https://bugs.webkit.org/show_bug.cgi?id=100173
> -    bool m_override : 1;
> -    bool m_hasHyphen : 1; // Used by BidiRun subclass which is a layering violation but enables us to save 8 bytes per object on 64-bit.
> -    unsigned char m_level;
> -    BidiCharacterRun* m_next;
> +private:
> +    std::unique_ptr<BidiCharacterRun> m_next;
> +
> +public:
>      int m_start;
>      int m_stop;
> +    unsigned char m_level;
> +    bool m_override : 1;
> +    bool m_hasHyphen : 1; // Used by BidiRun subclass which is a layering violation but enables us to save 8 bytes per object on 64-bit.

What is the size change after these changes?

> Source/WebCore/platform/text/BidiRunList.h:67
>      Run* m_lastRun;
>      Run* m_logicallyLastRun;

Would be nice to have comments saying who owns these raw pointers.

[Myles C. Maxfield]

Comment on attachment 275453 [details]
Patch

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

>> Source/WebCore/platform/text/BidiResolver.h:160
>> +        , m_override(context->override())
> 
> Why isn't m_level initialized here?

It is initialized on line 166 / 165 below.

>> Source/WebCore/platform/text/BidiResolver.h:197
>> +    bool m_hasHyphen : 1; // Used by BidiRun subclass which is a layering violation but enables us to save 8 bytes per object on 64-bit.
> 
> What is the size change after these changes?

It stays 24 bytes.

[Myles C. Maxfield]

Committed r198970: <http://trac.webkit.org/changeset/198970>

[Darin Adler]

Comment on attachment 275453 [details]
Patch

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

> Source/WebCore/platform/text/BidiRunList.h:182
> +    m_firstRun = nullptr;

There’s one significant difference from the old algorithm for deleting the linked list and the new one. This new algorithm uses recursion to delete the linked list, using stack proportional to the length of the list. The old algorithm used iteration instead. I don’t think that change is an improvement. This can be fixed in a couple ways. One way is to add this implementation of ~BidiCharacterRun instead of letting the compiler generate the default implementation:

    ~BidiCharacterRun()
    {
        // Delete the linked list in a loop to prevent destructor recursion.
        auto next = WTFMove(m_next);
        while (next)
            next = WTFMove(next->m_next);
    }

On the other hand, it’s possible that the recursive algorithm is acceptable, or even that the compiler is clever enough to turn this into tail recursion and optimize that to not use stack.

[Myles C. Maxfield]

Reopening to attach new patch.

[Myles C. Maxfield]

Created attachment 275561 [details]
Addressing Darin's comments

[Myles C. Maxfield]

Committed r199015: <http://trac.webkit.org/changeset/199015>