Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

This is a reland of https://codereview.chromium.org/1840163002.

Performance regressions were addressed in
https://codereview.chromium.org/1937613002
and https://codereview.chromium.org/1840163002.

BUG=596760, 599867
Committed: https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4
Cr-Commit-Position: refs/heads/master@{#395624}

[kotenkov, 2016-05-20 05:16:41]

kotenkov@yandex-team.ru changed reviewers:
+ haraken@chromium.org, yutak@chromium.org

[kotenkov, 2016-05-20 05:16:42]

It seems that we can try this again.
I've run some perftests, the only one that got through is this:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-05...
I've restarted android and mac perftests, will post results later.

[Yuta Kitamura, 2016-05-20 05:24:59]

yutak@chromium.org changed reviewers:
+ tkent@chromium.org

[Yuta Kitamura, 2016-05-20 05:25:00]

+tkent: Do you think it's okay to try this now?

[haraken, 2016-05-20 05:27:24]

On 2016/05/20 05:25:00, Yuta Kitamura wrote:
> +tkent: Do you think it's okay to try this now?

I'm asking it in https://codereview.chromium.org/1937613002/.

[tkent, 2016-05-20 05:34:47]

Do all performance tests run with OFFICIAL_BUILD?  If so, we can proceed.

[tkent, 2016-05-20 06:37:18]

On 2016/05/20 at 05:34:47, tkent wrote:
> Do all performance tests run with OFFICIAL_BUILD?  If so, we can proceed.

I confirmed all perf builder used official build.  e.g.
https://build.chromium.org/p/chromium.perf/builders/Android%20arm64%20Builder...

See also: https://codereview.chromium.org/1982123002

I think it's time to try this again.

[haraken, 2016-05-20 07:11:12]

On 2016/05/20 06:37:18, tkent wrote:
> On 2016/05/20 at 05:34:47, tkent wrote:
> > Do all performance tests run with OFFICIAL_BUILD?  If so, we can proceed.
> 
> I confirmed all perf builder used official build.  e.g.
>
https://build.chromium.org/p/chromium.perf/builders/Android%20arm64%20Builder...
> 
> See also: https://codereview.chromium.org/1982123002
> 
> I think it's time to try this again.

Yeah, looks solid. LGTM to try.

[Yuta Kitamura, 2016-05-20 07:12:56]

Mostly LG

https://codereview.chromium.org/1992873004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/dtoa/utils.h (right):

https://codereview.chromium.org/1992873004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/dtoa/utils.h:169: CHECK(index < length_);
CHECK_LE/LT?

https://codereview.chromium.org/1992873004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/StringConcatenate.h (right):

https://codereview.chromium.org/1992873004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/StringConcatenate.h:174: #if
defined(OFFICIAL_BUILD) && defined(NDEBUG)
Is there any reason this is guarded with
OFFICIAL_BUILD && NDEBUG? Doesn't CHECK() fail
unconditionally?

[kotenkov, 2016-05-24 07:50:06]

Here are some more perf results:
win10 blink_perf.layout:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-05...
nexus9 blink_perf.events:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-05...
I also got a compile error here:
https://build.chromium.org/p/tryserver.chromium.perf/builders/mac_hdd_perf_bi...
, but I cannot investigate it since I don't have access rights for the
https://build.chromium.org/p/tryserver.chromium.perf/builders/mac_hdd_perf_bi...
link (it seems that everything except html results on this page is google only).

https://codereview.chromium.org/1992873004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/dtoa/utils.h (right):

https://codereview.chromium.org/1992873004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/dtoa/utils.h:169: CHECK(index < length_);
On 2016/05/20 07:12:56, Yuta Kitamura wrote:
> CHECK_LE/LT?

Done.

https://codereview.chromium.org/1992873004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/StringConcatenate.h (right):

https://codereview.chromium.org/1992873004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/StringConcatenate.h:174: #if
defined(OFFICIAL_BUILD) && defined(NDEBUG)
On 2016/05/20 07:12:56, Yuta Kitamura wrote:
> Is there any reason this is guarded with
> OFFICIAL_BUILD && NDEBUG? Doesn't CHECK() fail
> unconditionally?

It does, but compiler doesn't have a way to see it, because CHECK() just calls a
function in the #else case here. These ifdefs are needed to avoid a compilation
error. Actually I think the NO_RETURN_DUE_TO_CRASH doesn't have much influence
on the performance, so we can remove it and have more concise and clear code.

[Yuta Kitamura, 2016-05-24 08:02:35]

OK, lgtm.

[kotenkov, 2016-05-24 15:13:46]

The CQ bit was checked by kotenkov@yandex-team.ru

[kotenkov, 2016-05-24 15:13:47]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1992873004/#ps40001
(title: "Specialized dchecks in utils.h.")

[commit-bot: I haz the power, 2016-05-24 15:14:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1992873004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1992873004/40001

[commit-bot: I haz the power, 2016-05-24 17:25:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-24 17:26:03]

Description was changed from

==========
Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

This is a reland of https://codereview.chromium.org/1840163002.

Performance regressions were addressed in
https://codereview.chromium.org/1937613002
and https://codereview.chromium.org/1840163002.

BUG=596760,599867
==========

to

==========
Replace all occurrences of RELEASE_ASSERT in wtf with CHECK.

This is a reland of https://codereview.chromium.org/1840163002.

Performance regressions were addressed in
https://codereview.chromium.org/1937613002
and https://codereview.chromium.org/1840163002.

BUG=596760,599867

Committed: https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4
Cr-Commit-Position: refs/heads/master@{#395624}
==========

[commit-bot: I haz the power, 2016-05-24 17:26:04]

Patchset 3 (id:??) landed as
https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4
Cr-Commit-Position: refs/heads/master@{#395624}

[kotenkov, 2016-05-27 06:13:54]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2016223002/ by kotenkov@yandex-team.ru.

The reason for reverting is: 12.5%-56.9% regression in blink_perf.layout at
395611:395672
http://crbug.com/614852.

[danakj, 2016-06-01 18:20:10]

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

[danakj, 2016-06-01 18:20:11]

https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/dtoa/utils.h (right):

https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/dtoa/utils.h:168: CHECK_LE(0, index);
It's a long shot but this isn't just a replacement of RELEASE_ASSERT with CHECK
since it uses 2 RELEASE_ASSERTs now essentially. Any chance this is the perf
regression?

[kotenkov, 2016-06-01 18:22:39]

https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/dtoa/utils.h (right):

https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/dtoa/utils.h:168: CHECK_LE(0, index);
On 2016/06/01 18:20:11, danakj wrote:
> It's a long shot but this isn't just a replacement of RELEASE_ASSERT with
CHECK
> since it uses 2 RELEASE_ASSERTs now essentially. Any chance this is the perf
> regression?

Hmm, I'll run perf try jobs later today.

[kotenkov, 2016-06-16 18:42:45]

On 2016/06/01 18:22:39, kotenkov wrote:
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/wtf/dtoa/utils.h (right):
> 
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/wtf/dtoa/utils.h:168: CHECK_LE(0, index);
> On 2016/06/01 18:20:11, danakj wrote:
> > It's a long shot but this isn't just a replacement of RELEASE_ASSERT with
> CHECK
> > since it uses 2 RELEASE_ASSERTs now essentially. Any chance this is the perf
> > regression?
> 
> Hmm, I'll run perf try jobs later today.

I've run the builds but I've completely forgotten to report the results. It
seems that the split of the CHECK in utils.h has nothing to do with the
regression.
In the following perftest results only RELEASE_ASSERT in utils.h is converted,
in the first one there is a single CHECK and in the second there are two:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...

In the following results all RELEASE_ASSERTs are converted and the CHECK in
utils.h is not split up:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...

As far as I can see, there is still a regression in
blink_perf.layout:floats_10_1000.

[danakj, 2016-06-16 18:45:12]

On 2016/06/16 18:42:45, kotenkov wrote:
> On 2016/06/01 18:22:39, kotenkov wrote:
> >
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/wtf/dtoa/utils.h (right):
> > 
> >
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/wtf/dtoa/utils.h:168: CHECK_LE(0, index);
> > On 2016/06/01 18:20:11, danakj wrote:
> > > It's a long shot but this isn't just a replacement of RELEASE_ASSERT with
> > CHECK
> > > since it uses 2 RELEASE_ASSERTs now essentially. Any chance this is the
perf
> > > regression?
> > 
> > Hmm, I'll run perf try jobs later today.
> 
> I've run the builds but I've completely forgotten to report the results. It
> seems that the split of the CHECK in utils.h has nothing to do with the
> regression.
> In the following perftest results only RELEASE_ASSERT in utils.h is converted,
> in the first one there is a single CHECK and in the second there are two:
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> 
> In the following results all RELEASE_ASSERTs are converted and the CHECK in
> utils.h is not split up:
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> 
> As far as I can see, there is still a regression in
> blink_perf.layout:floats_10_1000.

On the "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation"
thread, Nico pointed out his change to base was reverted, I think that must be
why this was a regression still? Can you confirm I read that right though?

[kotenkov, 2016-06-16 18:49:11]

On 2016/06/16 18:45:12, danakj wrote:
> On 2016/06/16 18:42:45, kotenkov wrote:
> > On 2016/06/01 18:22:39, kotenkov wrote:
> > >
> >
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/wtf/dtoa/utils.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/wtf/dtoa/utils.h:168: CHECK_LE(0, index);
> > > On 2016/06/01 18:20:11, danakj wrote:
> > > > It's a long shot but this isn't just a replacement of RELEASE_ASSERT
with
> > > CHECK
> > > > since it uses 2 RELEASE_ASSERTs now essentially. Any chance this is the
> perf
> > > > regression?
> > > 
> > > Hmm, I'll run perf try jobs later today.
> > 
> > I've run the builds but I've completely forgotten to report the results. It
> > seems that the split of the CHECK in utils.h has nothing to do with the
> > regression.
> > In the following perftest results only RELEASE_ASSERT in utils.h is
converted,
> > in the first one there is a single CHECK and in the second there are two:
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> > 
> > In the following results all RELEASE_ASSERTs are converted and the CHECK in
> > utils.h is not split up:
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> > 
> > As far as I can see, there is still a regression in
> > blink_perf.layout:floats_10_1000.
> 
> On the "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation"
> thread, Nico pointed out his change to base was reverted, I think that must be
> why this was a regression still? Can you confirm I read that right though?

If you are talking about this change: https://codereview.chromium.org/2046593002
,
this CL was reverted and the perftests were run when the change was still there.

[danakj, 2016-06-16 18:52:10]

danakj@chromium.org changed reviewers:
+ brettw@chromium.org

[danakj, 2016-06-16 18:52:11]

On 2016/06/16 18:49:11, kotenkov wrote:
> On 2016/06/16 18:45:12, danakj wrote:
> > On 2016/06/16 18:42:45, kotenkov wrote:
> > > On 2016/06/01 18:22:39, kotenkov wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/wtf/dtoa/utils.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/wtf/dtoa/utils.h:168: CHECK_LE(0, index);
> > > > On 2016/06/01 18:20:11, danakj wrote:
> > > > > It's a long shot but this isn't just a replacement of RELEASE_ASSERT
> with
> > > > CHECK
> > > > > since it uses 2 RELEASE_ASSERTs now essentially. Any chance this is
the
> > perf
> > > > > regression?
> > > > 
> > > > Hmm, I'll run perf try jobs later today.
> > > 
> > > I've run the builds but I've completely forgotten to report the results.
It
> > > seems that the split of the CHECK in utils.h has nothing to do with the
> > > regression.
> > > In the following perftest results only RELEASE_ASSERT in utils.h is
> converted,
> > > in the first one there is a single CHECK and in the second there are two:
> > >
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> > >
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> > > 
> > > In the following results all RELEASE_ASSERTs are converted and the CHECK
in
> > > utils.h is not split up:
> > >
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> > > 
> > > As far as I can see, there is still a regression in
> > > blink_perf.layout:floats_10_1000.
> > 
> > On the "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros
deprecation"
> > thread, Nico pointed out his change to base was reverted, I think that must
be
> > why this was a regression still? Can you confirm I read that right though?
> 
> If you are talking about this change:
https://codereview.chromium.org/2046593002
> ,
> this CL was reverted and the perftests were run when the change was still
there.

+thakis,brettw

So even with CHECK() doing a direct crash it regressed. What is going on :|

[danakj, 2016-06-16 18:56:51]

Ill inform the thread, so we can keep things together.

[danakj, 2016-06-16 18:58:13]

On 2016/06/16 18:49:11, kotenkov wrote:
> On 2016/06/16 18:45:12, danakj wrote:
> > On 2016/06/16 18:42:45, kotenkov wrote:
> > > On 2016/06/01 18:22:39, kotenkov wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/wtf/dtoa/utils.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1992873004/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/wtf/dtoa/utils.h:168: CHECK_LE(0, index);
> > > > On 2016/06/01 18:20:11, danakj wrote:
> > > > > It's a long shot but this isn't just a replacement of RELEASE_ASSERT
> with
> > > > CHECK
> > > > > since it uses 2 RELEASE_ASSERTs now essentially. Any chance this is
the
> > perf
> > > > > regression?
> > > > 
> > > > Hmm, I'll run perf try jobs later today.
> > > 
> > > I've run the builds but I've completely forgotten to report the results.
It
> > > seems that the split of the CHECK in utils.h has nothing to do with the
> > > regression.
> > > In the following perftest results only RELEASE_ASSERT in utils.h is
> converted,
> > > in the first one there is a single CHECK and in the second there are two:
> > >
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> > >
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> > > 
> > > In the following results all RELEASE_ASSERTs are converted and the CHECK
in
> > > utils.h is not split up:
> > >
> >
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> > > 
> > > As far as I can see, there is still a regression in
> > > blink_perf.layout:floats_10_1000.
> > 
> > On the "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros
deprecation"
> > thread, Nico pointed out his change to base was reverted, I think that must
be
> > why this was a regression still? Can you confirm I read that right though?
> 
> If you are talking about this change:
https://codereview.chromium.org/2046593002
> ,
> this CL was reverted and the perftests were run when the change was still
there.

Oh, I think brettw linked the wrong thing. Did these tests include
https://codereview.chromium.org/1982123002/ ?

[kotenkov, 2016-06-16 19:04:40]

On 2016/06/16 18:58:13, danakj wrote:
> Oh, I think brettw linked the wrong thing. Did these tests include
> https://codereview.chromium.org/1982123002/ ?

Yes, they did, they were run on June 1. Your change was introduced on May 17 and
reverted on June 6. The CL that we are in, was landed and reverted in between
these dates.

[danakj, 2016-06-16 21:11:41]

On 2016/06/16 19:04:40, kotenkov wrote:
> On 2016/06/16 18:58:13, danakj wrote:
> > Oh, I think brettw linked the wrong thing. Did these tests include
> > https://codereview.chromium.org/1982123002/ ?
> 
> Yes, they did, they were run on June 1. Your change was introduced on May 17
and
> reverted on June 6. The CL that we are in, was landed and reverted in between
> these dates.

Can you link to the bots that ran these tests? I'd like to try look at the bot
steps to confirm they are using official builds if possible.

[kotenkov, 2016-06-17 06:29:22]

On 2016/06/16 21:11:41, danakj wrote:
> On 2016/06/16 19:04:40, kotenkov wrote:
> > On 2016/06/16 18:58:13, danakj wrote:
> > > Oh, I think brettw linked the wrong thing. Did these tests include
> > > https://codereview.chromium.org/1982123002/ ?
> > 
> > Yes, they did, they were run on June 1. Your change was introduced on May 17
> and
> > reverted on June 6. The CL that we are in, was landed and reverted in
between
> > these dates.
> 
> Can you link to the bots that ran these tests? I'd like to try look at the bot
> steps to confirm they are using official builds if possible.

https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64ati_perf_...
https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64ati_perf_...
https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64intel_per...

[danakj, 2016-06-17 18:19:14]

On Thu, Jun 16, 2016 at 11:29 PM, <kotenkov@yandex-team.ru> wrote:

> On 2016/06/16 21:11:41, danakj wrote:
> > On 2016/06/16 19:04:40, kotenkov wrote:
> > > On 2016/06/16 18:58:13, danakj wrote:
> > > > Oh, I think brettw linked the wrong thing. Did these tests include
> > > > https://codereview.chromium.org/1982123002/ ?
> > >
> > > Yes, they did, they were run on June 1. Your change was introduced on
> May 17
> > and
> > > reverted on June 6. The CL that we are in, was landed and reverted in
> between
> > > these dates.
> >
> > Can you link to the bots that ran these tests? I'd like to try look at
> the bot
> > steps to confirm they are using official builds if possible.
>
>
>
https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64ati_perf_...
>
>
https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64ati_perf_...
>
>
https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64intel_per...
>

Aw, logs are gone by now. Well I looked at some other perf tryjobs and they
were using Official so that's good.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2016-06-17 18:19:14]

On Thu, Jun 16, 2016 at 11:29 PM, <kotenkov@yandex-team.ru> wrote:

> On 2016/06/16 21:11:41, danakj wrote:
> > On 2016/06/16 19:04:40, kotenkov wrote:
> > > On 2016/06/16 18:58:13, danakj wrote:
> > > > Oh, I think brettw linked the wrong thing. Did these tests include
> > > > https://codereview.chromium.org/1982123002/ ?
> > >
> > > Yes, they did, they were run on June 1. Your change was introduced on
> May 17
> > and
> > > reverted on June 6. The CL that we are in, was landed and reverted in
> between
> > > these dates.
> >
> > Can you link to the bots that ran these tests? I'd like to try look at
> the bot
> > steps to confirm they are using official builds if possible.
>
>
>
https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64ati_perf_...
>
>
https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64ati_perf_...
>
>
https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64intel_per...
>

Aw, logs are gone by now. Well I looked at some other perf tryjobs and they
were using Official so that's good.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.