New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 709832 link

Starred by 1 user
Status: Fixed
Owner:
ricea@chromium.org
Closed: Apr 2017
Cc:
domenic@chromium.org
█ tyoshino@chromium.org
Components:
EstimatedDays: ----
NextAction: 2017-04-21
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Feature



Sign in to add a comment

Remove (comment out) asserts in WritableStream.js

Project Member Reported by ricea@chromium.org, Apr 10 2017 
WritableStream.js has all the asserts from the standard included as TEMP_ASSERT statements. This is very useful for debugging, but has a significant performance impact.

They should be replaced by // assert() comments before M59 ships.
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 19 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ac2349b4fc1649f4fef0a69de517ad74acabc10

commit 8ac2349b4fc1649f4fef0a69de517ad74acabc10
Author: ricea <ricea@chromium.org>
Date: Wed Apr 19 09:14:32 2017

Comment out asserts in WritableStream.js

assert() statements can have a major performance impact, so comment them out.

They have not been removed completely as they can be easily re-enabled for
debugging purposes, and are also useful as documentation.

BUG= 709832 

Review-Url: https://codereview.chromium.org/2818963002
Cr-Commit-Position: refs/heads/master@{#465538}

[modify] https://crrev.com/8ac2349b4fc1649f4fef0a69de517ad74acabc10/third_party/WebKit/Source/core/streams/WritableStream.js

Comment 2 by ricea@chromium.org, Apr 20 2017

NextAction: 2017-04-21

Comment 3 by ricea@chromium.org, Apr 21 2017

Labels: Merge-Request-59 M-59 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Requesting merge to M-59 branch.

This change removes unnecessary debugging code, giving a useful performance benefit and reducing binary size.

Risk is extremely low, as there is no functional change. All modified functions are covered by tests.
Project Member

Comment 4 by sheriffbot@chromium.org, Apr 21 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 21 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f752fdbc7771c9fe61146f8f6c34c987f18430de

commit f752fdbc7771c9fe61146f8f6c34c987f18430de
Author: Adam Rice <ricea@chromium.org>
Date: Fri Apr 21 07:50:38 2017

Comment out asserts in WritableStream.js

assert() statements can have a major performance impact, so comment them out.

They have not been removed completely as they can be easily re-enabled for
debugging purposes, and are also useful as documentation.

BUG= 709832 

Review-Url: https://codereview.chromium.org/2818963002
Cr-Commit-Position: refs/heads/master@{#465538}
(cherry picked from commit 8ac2349b4fc1649f4fef0a69de517ad74acabc10)

Review-Url: https://codereview.chromium.org/2833033002 .
Cr-Commit-Position: refs/branch-heads/3071@{#116}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/f752fdbc7771c9fe61146f8f6c34c987f18430de/third_party/WebKit/Source/core/streams/WritableStream.js

Comment 6 by ricea@chromium.org, Apr 21 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment