New issue
Advanced search Search tips

Issue 807522 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug


Show other hotlists

Hotlists containing this issue:
Hotlist-9


Sign in to add a comment

Deprecate simpleBind from V8 Extras

Project Member Reported by bmeu...@chromium.org, Jan 31 2018

Issue description

The simpleBind function exposed by V8 Extras was initially added to work around the terrible performance of Function.prototype.bind at that time. Nowadays Function.prototype.bind is significantly faster and optimized fully by TurboFan, so there's no need to have simpleBind anymore.

Once the streams implementation is switched to Function#bind we can rip out simpleBind from V8.
 

Comment 1 by ricea@chromium.org, Jan 31 2018

I'm very happy to hear that bind's performance has been improved. Switching streams over should be as simple as

1. Take a copy of the initial value of bind, eg.

const Function_bind = v8.uncurryThis(global.Function.prototype.bind);

2. s/v8.simpleBind/Function_bind/
Looking at existing usages, I think we can just do

s/v8.simpleBind(x, y)/x.bind(y)/

because we only use it at startup time, so we don't need to get a clean copy via uncurryThis.

Comment 3 by ricea@chromium.org, Feb 1 2018

#2 Good point.
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 5 2018

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

commit 8ee5c39074ea39c67088feb73a01055e251b8b12
Author: Adam Rice <ricea@chromium.org>
Date: Mon Feb 05 10:12:28 2018

Streams: Stop using v8.simpleBind

v8.simpleBind() is being removed. Use Function.prototype.bind() in the
implementation of ReadableStream, WritableStream and TransformStream
instead. Microbenchmarks show no measurable difference in performance.

BUG= 807522 

Change-Id: Iba031f222dad5ccea6e97d4aa14483d1c453d853
Reviewed-on: https://chromium-review.googlesource.com/898828
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534358}
[modify] https://crrev.com/8ee5c39074ea39c67088feb73a01055e251b8b12/third_party/WebKit/Source/core/streams/CommonOperations.js
[modify] https://crrev.com/8ee5c39074ea39c67088feb73a01055e251b8b12/third_party/WebKit/Source/core/streams/ReadableStream.js
[modify] https://crrev.com/8ee5c39074ea39c67088feb73a01055e251b8b12/third_party/WebKit/Source/core/streams/TransformStream.js
[modify] https://crrev.com/8ee5c39074ea39c67088feb73a01055e251b8b12/third_party/WebKit/Source/core/streams/WritableStream.js

Cc: ricea@chromium.org domenic@chromium.org
Owner: mathias@chromium.org
Thanks Adam for fixing the Blink side. Assigning to Mathias for the V8 side.
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/fefee7dc1b4717deaa362ad293bfdea248d8ae42

commit fefee7dc1b4717deaa362ad293bfdea248d8ae42
Author: Mathias Bynens <mathias@chromium.org>
Date: Tue Feb 06 09:17:58 2018

[v8-extras] Remove `simpleBind`

The `simpleBind` function exposed by V8 Extras was initially added to
work around the terrible performance of `Function.prototype.bind` at
the time. Nowadays `Function.prototype.bind` is significantly faster
and fully optimized by TurboFan, however, so there’s no need for the
`simpleBind` helper anymore.

Bug:  chromium:807522 
Change-Id: I1a0456e2aa34f92a3c9a0234a812b660f969d016
Reviewed-on: https://chromium-review.googlesource.com/903164
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Mathias Bynens <mathias@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51108}
[modify] https://crrev.com/fefee7dc1b4717deaa362ad293bfdea248d8ae42/src/js/prologue.js
[modify] https://crrev.com/fefee7dc1b4717deaa362ad293bfdea248d8ae42/test/cctest/test-extra.js

Status: Fixed (was: Assigned)
Let’s file additional issues for any further V8 Extras cleanup. (For example, do we still need the Promise extras?)

Sign in to add a comment