Created attachment 211259 [details] [TEST] Reduction See attached test case. Wrapping in WebKit trunk differs from Safari 6.0.4 as well as Firefox and Chrome dev builds. TEST: > <div style="width: 3em; word-wrap: break-word;"> > 12345678 > <span>1</span><span>2</span><span>3</span><span>4</span><span>5</span><span>6</span><span>7</span><span>8</span> > </div> EXPECTED: > 123456 > 78 > 123456 > 78 ACTUAL: > 123456 > 78 > 1234567 > 8
<rdar://problem/14959559>
Easier test case (less looping through code!) > <div style="width: 3em; word-wrap: break-word;"> > 12345678<br> > 1234567<span>8</span> > </div> Zalan and Robert made changes in this area recently. I'll see if I can find when it regressed.
Interesting. It looks like behavior changed with r152793, a change by Zalan: <http://trac.webkit.org/changeset/152793>
So it looks like the "!current.m_pos" in this case might be mis-diagnosing input as an empty inline element? + } else if (!width.committedWidth() && (!current.m_obj || !current.m_obj->isBR()) && !current.m_pos) { + // Do not push the current object to the next line, when this line has some content, but it is still considered empty. + // Empty inline elements like <span></span> can produce such lines and now we just ignore these break opportunities + // at the start of a line, if no width has been committed yet. + // Behave as if it was actually empty and consume at least one object. + lBreak.increment(); } Zalan, any ideas?
(In reply to comment #4) > So it looks like the "!current.m_pos" in this case might be mis-diagnosing input as an empty inline element? > > + } else if (!width.committedWidth() && (!current.m_obj || !current.m_obj->isBR()) && !current.m_pos) { > + // Do not push the current object to the next line, when this line has some content, but it is still considered empty. > + // Empty inline elements like <span></span> can produce such lines and now we just ignore these break opportunities > + // at the start of a line, if no width has been committed yet. > + // Behave as if it was actually empty and consume at least one object. > + lBreak.increment(); > } > > Zalan, any ideas? I knew it was going to break something :( ...yes, it looks like a misdiagnosis of an "empty line" when we decide about the line breaking.
Created attachment 211712 [details] some more reduction
I suspect canBreakAtThisPosition() should allow breaking for "word-wrap: break-word;" when the content does not fit the line. For some reason, it currently returns false. I am looking into it.
I think I have the fix for this, patch is coming up soon.
Created attachment 212592 [details] Patch
Comment on attachment 212592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212592&action=review > Source/WebCore/rendering/LineWidth.cpp:158 > + UncommittedWidthMap::const_iterator it = m_uncommittedWidthMap.find(&object); > + if (it != m_uncommittedWidthMap.end()) > + return it->value; > + return -1; If this was returning 0 we could just use get, which would result in easier to read code. But I assume there’s a good reason we need to return -1 rather than 0. > Source/WebCore/rendering/LineWidth.cpp:169 > + UncommittedWidthMap::iterator it = m_uncommittedWidthMap.find(¤t); > + if (it == m_uncommittedWidthMap.end()) > + m_uncommittedWidthMap.add(¤t, delta); > + else > + it->value += delta; This is the way to write this efficiently: auto result = m_uncommittedWidthMap.add(¤t, delta); if (!result.isNewEntry) result.iterator->value += delta; Writing it this way does only a single hash table lookup, even the first time. > Source/WebCore/rendering/LineWidth.h:95 > + typedef HashMap<const RenderObject*, float> UncommittedWidthMap; > + UncommittedWidthMap m_uncommittedWidthMap; Our more modern style is to not use a typedef, and use “auto” instead when defining iterators. I suggest following that style instead. Is it really OK to add a map to every one of these? Even empty maps are relatively big objects.
> > Source/WebCore/rendering/LineWidth.cpp:169 > > + UncommittedWidthMap::iterator it = m_uncommittedWidthMap.find(¤t); > > + if (it == m_uncommittedWidthMap.end()) > > + m_uncommittedWidthMap.add(¤t, delta); > > + else > > + it->value += delta; > > This is the way to write this efficiently: > > auto result = m_uncommittedWidthMap.add(¤t, delta); > if (!result.isNewEntry) > result.iterator->value += delta; > > Writing it this way does only a single hash table lookup, even the first time. > > > Source/WebCore/rendering/LineWidth.h:95 > > + typedef HashMap<const RenderObject*, float> UncommittedWidthMap; > > + UncommittedWidthMap m_uncommittedWidthMap; > > Our more modern style is to not use a typedef, and use “auto” instead when defining iterators. I suggest following that style instead. > Thanks for the style/efficiency corrections above! I've already seen all of them (and used them), but apparently old reflexes came back. :( (In reply to comment #10) > (From update of attachment 212592 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212592&action=review > > > Source/WebCore/rendering/LineWidth.cpp:158 > > + UncommittedWidthMap::const_iterator it = m_uncommittedWidthMap.find(&object); > > + if (it != m_uncommittedWidthMap.end()) > > + return it->value; > > + return -1; > > If this was returning 0 we could just use get, which would result in easier to read code. But I assume there’s a good reason we need to return -1 rather than 0. There are objects (<br>) with 0 uncommitted length, so 0 is a valid value. > Is it really OK to add a map to every one of these? Even empty maps are relatively big objects. It's a stack allocated helper object in LineBreaker::nextSegmentBreak().
Created attachment 212696 [details] Patch
Comment on attachment 212696 [details] Patch EWS testing
Comment on attachment 212696 [details] Patch Clearing flags on attachment: 212696 Committed r156536: <http://trac.webkit.org/changeset/156536>
All reviewed patches have been landed. Closing bug.
This seems to have caused ~5% regression in inline layout performance: https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mountainlion%22%2C%22Layout%2Fline-layout%3ARuns%22%5D%5D&days=154&zoom=%5B1380171301992.8076%2C1380436865631.5427%5D I don't think it is a good idea to put a hash lookup into middle of this super hot code.
(In reply to comment #16) > This seems to have caused ~5% regression in inline layout performance: > > https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mountainlion%22%2C%22Layout%2Fline-layout%3ARuns%22%5D%5D&days=154&zoom=%5B1380171301992.8076%2C1380436865631.5427%5D > > I don't think it is a good idea to put a hash lookup into middle of this super hot code. Indeed. Let me find a different way to address this problem.
Comment on attachment 212696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212696&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:3286 > + } else if (!current.m_pos && !width.committedWidth() && current.m_obj && width.uncommittedWidthForObject(*current.m_obj) == width.uncommittedWidth()) { Basically the question is if there was any uncommitted width before processing the current object, right? Maybe a boolean to save that information on top of the loop would be enough?
It might be better to roll this out while pondering a better solution as it is bit invasive.
(In reply to comment #18) > (From update of attachment 212696 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212696&action=review > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:3286 > > + } else if (!current.m_pos && !width.committedWidth() && current.m_obj && width.uncommittedWidthForObject(*current.m_obj) == width.uncommittedWidth()) { > > Basically the question is if there was any uncommitted width before processing the current object, right? Maybe a boolean to save that information on top of the loop would be enough? It's a bit more involved than that, but yea, let's roll this out while finding a better fix.
rolled out http://trac.webkit.org/changeset/157100
Created attachment 214108 [details] Patch
Comment on attachment 214108 [details] Patch r=me
Comment on attachment 214108 [details] Patch Clearing flags on attachment: 214108 Committed r157391: <http://trac.webkit.org/changeset/157391>
Looks like this regressed line layout performance by 4-5% on Layout/line-layout:Runs: https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-lion%22%2C%22Layout%2Fline-layout%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Layout%2Fline-layout%3ARuns%22%5D%5D&days=56
(In reply to comment #26) > Looks like this regressed line layout performance by 4-5% on Layout/line-layout:Runs: > https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-lion%22%2C%22Layout%2Fline-layout%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Layout%2Fline-layout%3ARuns%22%5D%5D&days=56 I believe it was addressed by the rollout (you can see the perf recovery around 10/8) and a new fix was landed yesterday.