Unexpected word wrapping for wrapped content then raw content

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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

[Radar WebKit Bug Importer]

<rdar://problem/14959559>

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Interesting. It looks like behavior changed with r152793, a change by Zalan:
<http://trac.webkit.org/changeset/152793>

[Joseph Pecoraro]

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?

[zalan]

(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.

[zalan]

Created attachment 211712 [details]
some more reduction

[zalan]

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.

[zalan]

I think I have the fix for this, patch is coming up soon.

[zalan]

Created attachment 212592 [details]
Patch

[Darin Adler]

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(&current);
> +    if (it == m_uncommittedWidthMap.end())
> +        m_uncommittedWidthMap.add(&current, delta);
> +    else
> +        it->value += delta;

This is the way to write this efficiently:

    auto result = m_uncommittedWidthMap.add(&current, 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.

[zalan]

> > Source/WebCore/rendering/LineWidth.cpp:169
> > +    UncommittedWidthMap::iterator it = m_uncommittedWidthMap.find(&current);
> > +    if (it == m_uncommittedWidthMap.end())
> > +        m_uncommittedWidthMap.add(&current, delta);
> > +    else
> > +        it->value += delta;
> 
> This is the way to write this efficiently:
> 
>     auto result = m_uncommittedWidthMap.add(&current, 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().

[zalan]

Created attachment 212696 [details]
Patch

[zalan]

Comment on attachment 212696 [details]
Patch

EWS testing

[WebKit Commit Bot]

Comment on attachment 212696 [details]
Patch

Clearing flags on attachment: 212696

Committed r156536: <http://trac.webkit.org/changeset/156536>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

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.

[zalan]

(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.

[Antti Koivisto]

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?

[Antti Koivisto]

It might be better to roll this out while pondering a better solution as it is bit invasive.

[zalan]

(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.

[zalan]

rolled out http://trac.webkit.org/changeset/157100

[zalan]

Created attachment 214108 [details]
Patch

[Antti Koivisto]

Comment on attachment 214108 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 214108 [details]
Patch

Clearing flags on attachment: 214108

Committed r157391: <http://trac.webkit.org/changeset/157391>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[zalan]

(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.