RESOLVED FIXED 96042
Specialize nextBreakablePosition depending on breakNBSP
https://bugs.webkit.org/show_bug.cgi?id=96042
Summary Specialize nextBreakablePosition depending on breakNBSP
Benjamin Poulain
Reported 2012-09-06 17:12:34 PDT
This inner loop must be tighter!!!
Attachments
Patch (5.84 KB, patch)
2012-09-06 17:33 PDT, Benjamin Poulain
no flags
Patch (5.63 KB, patch)
2012-09-07 22:38 PDT, Benjamin Poulain
eric: review+
Benjamin Poulain
Comment 1 2012-09-06 17:33:26 PDT
Eric Seidel (no email)
Comment 2 2012-09-06 17:54:17 PDT
Comment on attachment 162630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162630&action=review That's a surprisingly small gain. Did you compare the instrument samples to see what the % change of this function is? > Source/WebCore/rendering/break_lines.cpp:149 > +template<> > +inline bool needsLineBreakIterator<false>(UChar ch) > { > return ch > asciiLineBreakTableLastChar && ch != noBreakSpace; > } Can't we write this as a single function? the compiler should optimize out useless branches you should jsut be able to have if (treatNoBreakSpaceAsBreak) in the shared implementation and have the compiler do the right thing? > Source/WebCore/rendering/break_lines.cpp:194 > +int nextBreakablePositionBreakOnNBSP(LazyLineBreakIterator& lazyBreakIterator, int pos) By OnNBSP? do you mean IncludingNBSP? I'm not quite sure what this function name means.
Benjamin Poulain
Comment 3 2012-09-06 18:12:08 PDT
> That's a surprisingly small gain. How much do you want? :) There are not really low hanging fruits left. > Did you compare the instrument samples to see what the % change of this function is? On the function itself, Instruments give ~9-10% faster. I guess could get better numbers with DTrace if you want exact numbers. > > Source/WebCore/rendering/break_lines.cpp:149 > > +template<> > > +inline bool needsLineBreakIterator<false>(UChar ch) > > { > > return ch > asciiLineBreakTableLastChar && ch != noBreakSpace; > > } > > Can't we write this as a single function? the compiler should optimize out useless branches you should jsut be able to have if (treatNoBreakSpaceAsBreak) in the shared implementation and have the compiler do the right thing? It could in one case but I prefer to be explicit. IMHO we should not rely blindly on compiler optimizations since this is a hotspot. > > Source/WebCore/rendering/break_lines.cpp:194 > > +int nextBreakablePositionBreakOnNBSP(LazyLineBreakIterator& lazyBreakIterator, int pos) > > By OnNBSP? do you mean IncludingNBSP? I'm not quite sure what this function name means. Yep indeed, this name is bad. nextBreakablePositionBreakIncludingBreakOnNBSP ? Long but explicit.
Eric Seidel (no email)
Comment 4 2012-09-06 20:30:47 PDT
(In reply to comment #3) > > That's a surprisingly small gain. > > How much do you want? :) > There are not really low hanging fruits left. > > > Did you compare the instrument samples to see what the % change of this function is? > > On the function itself, Instruments give ~9-10% faster. > > I guess could get better numbers with DTrace if you want exact numbers. Nah, that's fine. :) I remember seeing this function as being about 5% of total time. With your change I would expect it to be much less than 5% if we're seeing a 3% overall win. > > > Source/WebCore/rendering/break_lines.cpp:149 > > > +template<> > > > +inline bool needsLineBreakIterator<false>(UChar ch) > > > { > > > return ch > asciiLineBreakTableLastChar && ch != noBreakSpace; > > > } > > > > Can't we write this as a single function? the compiler should optimize out useless branches you should jsut be able to have if (treatNoBreakSpaceAsBreak) in the shared implementation and have the compiler do the right thing? > > It could in one case but I prefer to be explicit. > IMHO we should not rely blindly on compiler optimizations since this is a hotspot. I guess. :) It would be an extremely dumb compiler to not remove unreachable code! > > > Source/WebCore/rendering/break_lines.cpp:194 > > > +int nextBreakablePositionBreakOnNBSP(LazyLineBreakIterator& lazyBreakIterator, int pos) > > > > By OnNBSP? do you mean IncludingNBSP? I'm not quite sure what this function name means. > > Yep indeed, this name is bad. > > nextBreakablePositionBreakIncludingBreakOnNBSP ? > Long but explicit. Thinking about it more, I might just go the other way. Have the "including NBSP" one just be "nextBreakablePosition" and the non-NBSP one be "nextBreakablePositionIgnoringNBSP". Your call.
Benjamin Poulain
Comment 5 2012-09-07 22:14:47 PDT
> > It could in one case but I prefer to be explicit. > > IMHO we should not rely blindly on compiler optimizations since this is a hotspot. > > I guess. :) It would be an extremely dumb compiler to not remove unreachable code! Reading back, I realize I totally misunderstood you. I originally thought you were asking to rewrite this in a way that the compiler know the ch test is useless. I figured, sure, that is gonna be tricky but should be possible. :) Now that I see what you really asked... sure, I can change to if (treatNoBreakSpaceAsBreak) :-D > Thinking about it more, I might just go the other way. Have the "including NBSP" one just be "nextBreakablePosition" and the non-NBSP one be "nextBreakablePositionIgnoringNBSP". Your call. Changing to nextBreakablePositionIgnoringNBSP() is ok with me.
Benjamin Poulain
Comment 6 2012-09-07 22:38:01 PDT
Eric Seidel (no email)
Comment 7 2012-09-07 23:03:34 PDT
Comment on attachment 162942 [details] Patch Assuming this is still a similar win, sounds great!
Benjamin Poulain
Comment 8 2012-09-08 17:35:27 PDT
Benjamin Poulain
Comment 9 2012-09-10 16:40:02 PDT
The chrome bots get the expected improvements: http://webkit-perf.appspot.com/graph.html#tests=[[2947022,2001,3001]]&sel=none&displayrange=7&datatype=running The Mac ones do not: http://webkit-perf.appspot.com/graph.html#tests=[[2947022,2001,32196]]&sel=none&displayrange=7&datatype=running Locally, on Mac, I get the same results as the Chrome bots. I have no explanation for the bot results. :(
Note You need to log in before you can comment on or make changes to this bug.