In TextBreakIteratorGtk.cpp, function setUpIterator(), line 68-69, the m_logAttrs are always freed, because createdIterator is set true in line 55.
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.
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.
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.
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.
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!
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