-webkit-text-underline-position support was implemented in [1]. Eric Seidel has some concerns about memory usage [2] because of the private variable m_maxLogicalTop added to all RootInlineBox objects. This bug intents to improve memory usage of -webkit-text-underline-position by adding a cache for m_maxLogicalTop that will be used only when -webkit-text-underline-position is used in the page. That way if the page does not use -webkit-text-underline-position it should not hit memory overhead. [1] http://trac.webkit.org/changeset/146104 [2] https://codereview.chromium.org/14576017#msg13
Hi, opening a complex website creates about 2000 RootInlineBoxes, then the m_maxLogicalTop variable for all of them takes up about 16 KB (8 bytes per RootInlineBox). On the other hand if I just call computeMaxLogicalTop() everytime RootInlineBox::maxLogicalTop() is called it takes from 1 to 3 microseconds to run in my laptop. That looks pretty fast and I am not sure if implementing a cache is worthy now.
Created attachment 201968 [details] Patch Proposed patch. Change RootInlineBox::maxLogicalTop() to compute the maxLogicalTop everytime it is called. The cpu power needed to compute maxLogicalTop is small and by removing m_maxLogicalTop variable we can reduce the memory used to store RootInlineBoxes.
Comment on attachment 201968 [details] Patch Attachment 201968 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/480494
Comment on attachment 201968 [details] Patch Attachment 201968 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/486092
Comment on attachment 201968 [details] Patch Attachment 201968 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/486095
Created attachment 201972 [details] Patch Fix build with --css3-text disabled and remove obsolete comment.
Comment on attachment 201972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201972&action=review The patch looks like a good idea but I have concerns about constness. Either those functions are badly named, or they should be const. > Source/WebCore/rendering/RootInlineBox.cpp:345 > +float RootInlineBox::maxLogicalTop() > +{ > + float maxLogicalTop = 0; > + computeMaxLogicalTop(maxLogicalTop); > + return maxLogicalTop; > +} > +#endif // CSS3_TEXT Looks like computeMaxLogicalTop() should be const. Then maxLogicalTop() should be const too.
Created attachment 201998 [details] Patch Make computeMaxLogicalTop and maxLogicalTop const.
Comment on attachment 201998 [details] Patch LGTM.
Comment on attachment 201998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201998&action=review We should add compile-time size assertions for RootInlineBox and friends so this doesn’t happen again. > Source/WebCore/ChangeLog:11 > + the maxLogicalTop value everytime it is called. In a tipical page the typical > Source/WebCore/ChangeLog:12 > + number of interactions to calcule it is less than 10, so it is a small ‘interactions’ is an odd word to use here. Calls? Also, calculate.
(In reply to comment #10) > (From update of attachment 201998 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201998&action=review > > We should add compile-time size assertions for RootInlineBox and friends so this doesn’t happen again. I will submit a patch for that. > > Source/WebCore/ChangeLog:12 > > + number of interactions to calcule it is less than 10, so it is a small > > ‘interactions’ is an odd word to use here. Calls? Also, calculate. It should have been "iterations", I wrote that word because of the "for" loop in computeMaxLogicalTop, but "calls" looks better. What I want to explain is that computeMaxLogicalTop needs to iterate through all descendants of the RootInlineBox. The number of descendants is usually very small (less than 10) by what I have checked.
Created attachment 202081 [details] Patch Update ChangeLog.
Comment on attachment 202081 [details] Patch Rejecting attachment 202081 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--non-interactive', 202081, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.appspot.com/results/487419
(In reply to comment #13) > (From update of attachment 202081 [details]) > Rejecting attachment 202081 [details] from commit-queue. > > Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--non-interactive', 202081, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit > > /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). > > Full output: http://webkit-queues.appspot.com/results/487419 Oops :P Lamarque, can you please update the "Reviewed by" section from ChangeLog?
Created attachment 202086 [details] Patch Update ChangeLog.
Comment on attachment 202086 [details] Patch CQ'ing again :)
Comment on attachment 202086 [details] Patch Clearing flags on attachment: 202086 Committed r150258: <http://trac.webkit.org/changeset/150258>