Fix problems with flakes in SVG LayoutTests by reordering arithmetic.

Fix problems with flakes in SVG LayoutTests by reordering arithmetic.

This operation gives 4.99999952: float(25 + 200 * double(1.f*(-10.0f/100.f)))
This operation gives 5.00000000: float(25 + 200 * float(1.f*(-10.0f/100.f)))

Why 4.99999952 appear only sometimes, and only on Windows, is
not fully known. A suspect is that it's related to the 32-bit ABI which
requires use of x87 for passing floats. That combined with various
inlining decisions and context switches could possibly affect the final
value in a way that makes it seem random.

The fix in this patch is to reorder some operations. Using the example
above it will be changed to:

  float(25 + (200 * 1.f * -10.f) / 100.f)

By delaying the dangerous division, every intermediate value will
be representable by a float with no errors.

This won't solve all floating point errors but it will solve the one that causes
the most common CQ flakes.

BUG=438282
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188871

[Daniel Bratell, 2015-01-21 15:58:24]

Patchset #1 (id:1) has been deleted

[Daniel Bratell, 2015-01-21 17:44:27]

bratell@opera.com changed reviewers:
+ fs@opera.com, pdr@chromium.org

[Daniel Bratell, 2015-01-21 17:44:28]

This is a non-strange change to make the flakiness go away.
It solves the case when an SVG is using an integral number of percentage. It
wouldn't solve the case where you write 0.1 instead of 10%.

pdr and fs, can you take a look, please?

(I expect to have to change a few more expect files but otherwise I think it's
done).

[Daniel Bratell, 2015-01-22 10:28:26]

On 2015/01/21 17:44:28, Daniel Bratell wrote:
> This is a non-strange change to make the flakiness go away.
> It solves the case when an SVG is using an integral number of percentage. It
> wouldn't solve the case where you write 0.1 instead of 10%.
> 
> pdr and fs, can you take a look, please?
> 
> (I expect to have to change a few more expect files but otherwise I think it's
> done).

All trybots pass now so I think I've updated all relevant tests.

[fs, 2015-01-22 12:37:36]

Personally I think I'd prefer to just do the "/ 100" explicitly where necessary
(since we tend to know the type anyway etc.), but ISTR that pdr would like there
to be only normalized values (factors) where there are percentages today... =)

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.cpp:206: float SVGLength::scaleByPercentage(float
input) const
Given the specific name - but generic function - maybe this should be scaleBy()
or only have the percentage-branch (and assert that type==percentage).

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:72: // Resolves LengthTypePercentage into a normal
floating point number (full value is 1.0).
s/normal/normalized/ ?

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:209: ASSERT_NOT_REACHED();
I'd prefer to structure this new function like the previous - i.e. tail-calls
and ASSERT_NOT_REACHED() outside the switch.

[Daniel Bratell, 2015-01-22 13:13:44]

Please take a new look.

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.cpp:206: float SVGLength::scaleByPercentage(float
input) const
On 2015/01/22 12:37:36, fs wrote:
> Given the specific name - but generic function - maybe this should be
scaleBy()
> or only have the percentage-branch (and assert that type==percentage).

Judging from how the code was used, it seems code handled "0.1" and "10%" the
same. There is for instance the code below that handles that only one of "from"
and "to" has a percentage sign and the other does not. It would be different
(more cases) if the difference was exposed.

For this code in particular, I just can't see if resolveRectangle is only called
with all of width, height, x and y are percentages.

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:72: // Resolves LengthTypePercentage into a normal
floating point number (full value is 1.0).
On 2015/01/22 12:37:36, fs wrote:
> s/normal/normalized/ ?

Done.

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:209: ASSERT_NOT_REACHED();
On 2015/01/22 12:37:36, fs wrote:
> I'd prefer to structure this new function like the previous - i.e. tail-calls
> and ASSERT_NOT_REACHED() outside the switch.

Ok.

I do like this structure though because it generates more efficient code and
won't contain dummy return values put there to silence compilers, but instead
causing confusion in code analyzers.

[fs, 2015-01-22 14:06:36]

LGTM, but please allow for pdr to have a look too.

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.cpp:206: float SVGLength::scaleByPercentage(float
input) const
On 2015/01/22 13:13:44, Daniel Bratell wrote:
> On 2015/01/22 12:37:36, fs wrote:
> > Given the specific name - but generic function - maybe this should be
> scaleBy()
> > or only have the percentage-branch (and assert that type==percentage).
> 
> Judging from how the code was used, it seems code handled "0.1" and "10%" the
> same. There is for instance the code below that handles that only one of
"from"
> and "to" has a percentage sign and the other does not. It would be different
> (more cases) if the difference was exposed.
> 
> For this code in particular, I just can't see if resolveRectangle is only
called
> with all of width, height, x and y are percentages.

Yes, that's true... I suppose I'm still used to percentage being resolved in a
different way.

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/861213003/diff/120001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:209: ASSERT_NOT_REACHED();
On 2015/01/22 13:13:44, Daniel Bratell wrote:
> On 2015/01/22 12:37:36, fs wrote:
> > I'd prefer to structure this new function like the previous - i.e.
tail-calls
> > and ASSERT_NOT_REACHED() outside the switch.
> 
> Ok.
> 
> I do like this structure though because it generates more efficient code and
> won't contain dummy return values put there to silence compilers, but instead
> causing confusion in code analyzers.

Well saying that it generates better code is a bold statement IMO =) (plenty of
things to fubar in this version too.) And the dummy return value is instead
replaced by a dummy switch-case. (Also, if ASSERT_NOT_REACHED is implemented in
a meaningful way, that edge will be dead in practice.)

https://codereview.chromium.org/861213003/diff/140001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/861213003/diff/140001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:206: float resultValue;
Nit: Unused.

https://codereview.chromium.org/861213003/diff/140001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:214: };
Nit: Redundant ;

[Daniel Bratell, 2015-01-22 14:15:46]

https://codereview.chromium.org/861213003/diff/140001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/861213003/diff/140001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:206: float resultValue;
On 2015/01/22 14:06:36, fs wrote:
> Nit: Unused.

Done.

https://codereview.chromium.org/861213003/diff/140001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthContext.cpp:214: };
On 2015/01/22 14:06:36, fs wrote:
> Nit: Redundant ;

Done.

[Daniel Bratell, 2015-01-22 14:16:41]

pdr, if you don't mind this change, then feel free to use the Commit checkbox.
It's the most common flake by far so if this fixes things (I think they will),
it will make people very happy.

[Stephen Chennney, 2015-01-22 21:57:24]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2015-01-22 21:57:25]

These are old comments I had when I looked at it. I don't know why they weren't
sent sooner (and the line numbers might be wrong).

pdr might be slow to review.

https://codereview.chromium.org/861213003/diff/80001/Source/core/svg/SVGLengt...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/861213003/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLength.cpp:188: // 100% = 100.0 instead of 1.0 for historical
reasons, this could eventually be changed
This comment seems wrong now.

https://codereview.chromium.org/861213003/diff/80001/Source/core/svg/SVGLengt...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/861213003/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLengthContext.cpp:204: float
SVGLengthContext::convertValueFromPercentageToUserUnits(const SVGLength& value,
const FloatSize& viewportSize)
Please either move this to be with the other method (that takes a float) or move
the other one here. If they basically do the same thing, I would like them to be
beside each other.

[Daniel Bratell, 2015-01-23 08:49:40]

https://codereview.chromium.org/861213003/diff/80001/Source/core/svg/SVGLengt...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/861213003/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLength.cpp:188: // 100% = 100.0 instead of 1.0 for historical
reasons, this could eventually be changed
On 2015/01/22 21:57:25, Stephen Chenney wrote:
> This comment seems wrong now.

It is still correct since it refers to the internal representation of
LengthTypePercentage. I will rephrase them though, so that it's more clear.

https://codereview.chromium.org/861213003/diff/80001/Source/core/svg/SVGLengt...
File Source/core/svg/SVGLengthContext.cpp (right):

https://codereview.chromium.org/861213003/diff/80001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLengthContext.cpp:204: float
SVGLengthContext::convertValueFromPercentageToUserUnits(const SVGLength& value,
const FloatSize& viewportSize)
On 2015/01/22 21:57:25, Stephen Chenney wrote:
> Please either move this to be with the other method (that takes a float) or
move
> the other one here. If they basically do the same thing, I would like them to
be
> beside each other.

I think this comment was based on some other version of the code because they
are together.

[fs, 2015-01-23 09:05:18]

Still LGTM

[Daniel Bratell, 2015-01-23 09:27:41]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2015-01-23 09:28:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/861213003/200001

[commit-bot: I haz the power, 2015-01-23 10:09:15]

Committed patchset #10 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188871

[pdr., 2015-01-23 21:59:27]

On 2015/01/22 at 14:16:41, bratell wrote:
> pdr, if you don't mind this change, then feel free to use the Commit checkbox.
It's the most common flake by far so if this fixes things (I think they will),
it will make people very happy.

I apologize for being slow. Had jury duty earlier this week (mandatory civil
service in the US, also may continue for the next month, TBD). This change looks
great, LGTM