Bug 42759 – Always freeing m_logAttrs in TextBreakIteratorGtk.cpp: setUpIterator()

• Status: RESOLVED
• Resolution: REMIND
• Component: Text

[Jens]

In TextBreakIteratorGtk.cpp, function setUpIterator(), line 68-69, the m_logAttrs are always freed, because createdIterator is set true in line 55.

[Peter Hatina]

Created attachment 92060 [details]
m_logAttrs freed when necessary

In the static function setUpIterator(...), the iterator public member m_logAttrs is freed and reallocated, when its length isn't sufficient for the proposed string. Otherwise, no allocation + deallocation is called and we can use part of the former array m_logAttrs.

[Martin Robinson]

Comment on attachment 92060 [details]
m_logAttrs freed when necessary

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

Thanks for your contribution! What's your motivation for this change? Does it provide a measurable performance improvement in breaking text? 

This patch is missing a ChangeLog entry, which is a requirement for all WebKit patches. (see http://www.webkit.org/coding/contributing.html). I recommend using the ./Tools/Scripts/webkit-patch tool to submit your patches. It will warn you about style violations before you even submit your patch. You can also use check-webkit-style. Here are the coding style guidelines: http://www.webkit.org/coding/coding-style.html

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:194
> +    int m_logAttrsLen;

Should be m_logAttrsLength.

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:200
> +    , m_logAttrs(NULL)

In WebKit we use 0 instead of NULL except in situations where it would cause an error or warning.

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:231
> +    } else {
> +        memset(static_cast<void*>(iterator->m_logAttrs), 0, charLength + 1);
> +    }

Since you have only one statement in this block, the style guide says you must omit the curly braces.

[Peter Hatina]

Created attachment 92237 [details]
m_logAttrs freed when necessary

Wrt motivation: When breaking the text, I think, it is not always necessary to reallocate the memory block, if it is sufficient for future use. Also we save mem. allocation call and part of zero-ing proces, which is done only on the part of the portion. To my mind, it is also cheaper to use once-prepared block, than always release it and call for another one.

Wrt coding style: now it should be fine.

[Martin Robinson]

Comment on attachment 92237 [details]
m_logAttrs freed when necessary

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

Seems okay. But really m_logAttrs should be a GOwnPtr if possible.

> Source/WebCore/ChangeLog:17
> +        When breaking the text, I think, it is not always necessary to reallocate the memory
> +        block, if it is sufficient for future use. Also we save a mem. allocation call and part
> +        of the zeroing proces, which is done only on the part of the portion. To my mind,
> +        it is also cheaper to use once-prepared block, than always release it and call
> +        for another one.

Feel free to just say "Avoid an unecessary memory allocation, if we can." or something like that. :)

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:193
>      PangoLogAttr* m_logAttrs;

Might as well change this to a GOwnPtr<PangoLogAttr> now. You can do away with all calls to g_free then. Right now it looks like m_logAttrs just leaks!

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:205
> +TextBreakIterator::TextBreakIterator()
> +    : m_type(UBRK_CHARACTER)
> +    , m_logAttrs(0)
> +    , m_logAttrsLength(0)
> +    , m_charIterator()
> +{
> +}
> +

Hrm. I believe you should either pass these parameters to the constructor and itialize them or initialize them externally. Not both though.

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:231
> +        iterator->m_logAttrs = g_new0(PangoLogAttr, charLength + 1);
> +        iterator->m_logAttrsLength = charLength + 1;
> +    } else
> +        memset(static_cast<void*>(iterator->m_logAttrs), 0, charLength + 1);
> +

Since you always need to do memset you can simplify this:

if (createdIterator) {
    iterator->m_logAttrs = g_new(PangoLogAttr, charLength + 1);
    iterator->m_logAttrsLength = charLength + 1;
}
memset(static_cast<void*>(iterator->m_logAttrs), 0, charLength + 1);

I'm still curious whether this has any measurable performance ramifications.

[Peter Hatina]

After real tests, this bug seems to be not relevant. Freeing the memory block when necessary makes the code faster only few 0.003s faster. Now I am talking about the page having at least 20000 words in its content. I thought, Jens had a real reason, why to do this.

I suggest to switch this to the INVALID status. Thank you Martin!

[Jens]

Hello All

Thank you for taking care of this.

(In reply to comment #5)
> After real tests, this bug seems to be not relevant. Freeing the memory block when necessary makes the code faster only few 0.003s faster. Now I am talking about the page having at least 20000 words in its content. I thought, Jens had a real reason, why to do this.

I just wanted someone to tell me if this (revision 71296)

213    if (createdIterator)
214        g_free(iterator->m_logAttrs);

could be replaced by this

213    g_free(iterator->m_logAttrs);

(because createdIterator is always true there) or if I missed something (e.g. createdIterator should not be set true in line 202).
I did not have performance in mind. 

> I suggest to switch this to the INVALID status. Thank you Martin!

Best regards
Jens