Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

broken test-wasm-simple #24

Closed
targos opened this issue Nov 19, 2017 · 4 comments
Closed

broken test-wasm-simple #24

targos opened this issue Nov 19, 2017 · 4 comments

Comments

@targos
Copy link
Member

targos commented Nov 19, 2017

Probably caused by https://chromium-review.googlesource.com/c/v8/v8/+/775338
@targos targos changed the title test-wasm-simple broken broken test-wasm-simple Nov 19, 2017
@bmeurer
Copy link
Member

bmeurer commented Nov 19, 2017

cc @gahaas

@addaleax
Copy link
Member

Heh, yes. The test for Node was actually suggested by me with this specific feature in mind.

I’m surprised to see V8 extended its platform API to accommodate this – I am pretty sure I had made the preparations for this to work with the old API (i.e. I’m not sure why that extension was necessary).

Anyway, I’m taking a look at the crash itself now.

@addaleax addaleax mentioned this issue Nov 19, 2017
Closed
3 tasks
@addaleax
Copy link
Member

Fix is in nodejs/node#17134

@gahaas
Copy link

gahaas commented Nov 20, 2017

Hi Anna,

Thanks for addressing this issue so quickly.

Let me quickly explain the reason for the platform API change: There is a fundamental assumption in V8 that the isolate is only used by foreground tasks. The old platform API required the isolate to post new foreground task. In principle this meant that only foreground tasks were able to post new foreground tasks. This was, however insufficient for asynchronous compilation of WebAssembly.

With the new platform API a foreground task can create a TaskRunner using the isolate, and then a background task can use that TaskRunner to post new foreground tasks without using the isolate.

Cheers, Andreas

addaleax added a commit to addaleax/node that referenced this issue Nov 21, 2017
@addaleax
src: implement v8::TaskRunner API in NodePlatform
8be600b 
V8 is switching APIs for scheduling tasks. Implement the new APIs.

Fixes: nodejs/node-v8#24
Refs: v8/v8@c690f54
jasnell pushed a commit to nodejs/node that referenced this issue Nov 21, 2017
@addaleax @jasnell
deps: cherry-pick 98c40a4bae915 from V8 upstream
2540e3d 
Original commit message:

    [platform] Return task runners as shared_ptr

    At the moment, task runners are returned as unique_ptr. This is
    inconvenient, however. In all implementations I did, the platform holds
    a shared pointer of the task runner and wraps it in a wrapper class just
    to return it as a unique_ptr. With this CL the platform API is changed
    to return a shared_ptr directly.

    R=rmcilroy@chromium.org

    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ide278db855199ea239ad0ae14d97fd17349dac8c
    Reviewed-on: https://chromium-review.googlesource.com/768867
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#49366}

Refs: v8/v8@98c40a4

PR-URL: #17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit to nodejs/node that referenced this issue Nov 21, 2017
@addaleax @jasnell
src: implement v8::TaskRunner API in NodePlatform
f005656 
V8 is switching APIs for scheduling tasks. Implement the new APIs.

Fixes: nodejs/node-v8#24
Refs: v8/v8@c690f54

PR-URL: #17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Dec 6, 2017
@addaleax @targos
deps: cherry-pick c690f54d95802 from V8 upstream
ae4a741 
Original commit message:

    [platform] Add TaskRunner to the platform API

    With the existing platform API it is not possible to post foreground
    tasks from background tasks. This is, however, required to implement
    asynchronous compilation for WebAssembly. With this CL we add the
    concept of a TaskRunner to the platform API. The TaskRunner contains
    all data needed to post a foreground task and can be used both from a
    foreground task and a background task. Eventually the TaskRunner should
    replace the existing API.

    In addition, this CL contains a default implementation of the
    TaskRunner. This implementation has tempory workaround for platforms
    which do not provide a TaskRunner implementation yet. This default
    implementation should be deleted again when all platforms provide a
    TaskRunner implementation.

    R=rmcilroy@chromium.org

    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I6ea4a1c9da1eb9a19e8ce8f2163000dbc2598802
    Reviewed-on: https://chromium-review.googlesource.com/741588
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#49041}

Refs: v8/v8@c690f54

PR-URL: nodejs#17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Dec 6, 2017
@addaleax @targos
deps: cherry-pick 98c40a4bae915 from V8 upstream
d7a8a5c 
Original commit message:

    [platform] Return task runners as shared_ptr

    At the moment, task runners are returned as unique_ptr. This is
    inconvenient, however. In all implementations I did, the platform holds
    a shared pointer of the task runner and wraps it in a wrapper class just
    to return it as a unique_ptr. With this CL the platform API is changed
    to return a shared_ptr directly.

    R=rmcilroy@chromium.org

    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ide278db855199ea239ad0ae14d97fd17349dac8c
    Reviewed-on: https://chromium-review.googlesource.com/768867
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#49366}

Refs: v8/v8@98c40a4

PR-URL: nodejs#17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Dec 6, 2017
@addaleax @targos
deps: cherry-pick c690f54d95802 from V8 upstream
99ac5cd 
Original commit message:

    [platform] Add TaskRunner to the platform API

    With the existing platform API it is not possible to post foreground
    tasks from background tasks. This is, however, required to implement
    asynchronous compilation for WebAssembly. With this CL we add the
    concept of a TaskRunner to the platform API. The TaskRunner contains
    all data needed to post a foreground task and can be used both from a
    foreground task and a background task. Eventually the TaskRunner should
    replace the existing API.

    In addition, this CL contains a default implementation of the
    TaskRunner. This implementation has tempory workaround for platforms
    which do not provide a TaskRunner implementation yet. This default
    implementation should be deleted again when all platforms provide a
    TaskRunner implementation.

    R=rmcilroy@chromium.org

    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I6ea4a1c9da1eb9a19e8ce8f2163000dbc2598802
    Reviewed-on: https://chromium-review.googlesource.com/741588
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#49041}

Refs: v8/v8@c690f54

PR-URL: #17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Dec 6, 2017
@addaleax @targos
deps: cherry-pick 98c40a4bae915 from V8 upstream
3d43bce 
Original commit message:

    [platform] Return task runners as shared_ptr

    At the moment, task runners are returned as unique_ptr. This is
    inconvenient, however. In all implementations I did, the platform holds
    a shared pointer of the task runner and wraps it in a wrapper class just
    to return it as a unique_ptr. With this CL the platform API is changed
    to return a shared_ptr directly.

    R=rmcilroy@chromium.org

    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ide278db855199ea239ad0ae14d97fd17349dac8c
    Reviewed-on: https://chromium-review.googlesource.com/768867
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#49366}

Refs: v8/v8@98c40a4

PR-URL: #17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 12, 2017
@addaleax @MylesBorins
deps: cherry-pick c690f54d95802 from V8 upstream
7812c93 
Original commit message:

    [platform] Add TaskRunner to the platform API

    With the existing platform API it is not possible to post foreground
    tasks from background tasks. This is, however, required to implement
    asynchronous compilation for WebAssembly. With this CL we add the
    concept of a TaskRunner to the platform API. The TaskRunner contains
    all data needed to post a foreground task and can be used both from a
    foreground task and a background task. Eventually the TaskRunner should
    replace the existing API.

    In addition, this CL contains a default implementation of the
    TaskRunner. This implementation has tempory workaround for platforms
    which do not provide a TaskRunner implementation yet. This default
    implementation should be deleted again when all platforms provide a
    TaskRunner implementation.

    R=rmcilroy@chromium.org

    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I6ea4a1c9da1eb9a19e8ce8f2163000dbc2598802
    Reviewed-on: https://chromium-review.googlesource.com/741588
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#49041}

Refs: v8/v8@c690f54

PR-URL: #17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 12, 2017
@addaleax @MylesBorins
deps: cherry-pick 98c40a4bae915 from V8 upstream
0411572 
Original commit message:

    [platform] Return task runners as shared_ptr

    At the moment, task runners are returned as unique_ptr. This is
    inconvenient, however. In all implementations I did, the platform holds
    a shared pointer of the task runner and wraps it in a wrapper class just
    to return it as a unique_ptr. With this CL the platform API is changed
    to return a shared_ptr directly.

    R=rmcilroy@chromium.org

    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ide278db855199ea239ad0ae14d97fd17349dac8c
    Reviewed-on: https://chromium-review.googlesource.com/768867
    Commit-Queue: Andreas Haas <ahaas@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#49366}

Refs: v8/v8@98c40a4

PR-URL: #17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 12, 2017
@addaleax @MylesBorins
src: implement v8::TaskRunner API in NodePlatform
936c0b2 
V8 is switching APIs for scheduling tasks. Implement the new APIs.

Fixes: nodejs/node-v8#24
Refs: v8/v8@c690f54

PR-URL: #17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
blattersturm pushed a commit to citizenfx/node that referenced this issue Nov 3, 2018
@addaleax @blattersturm
src: implement v8::TaskRunner API in NodePlatform
49831f9 
V8 is switching APIs for scheduling tasks. Implement the new APIs.

Fixes: nodejs/node-v8#24
Refs: v8/v8@c690f54

PR-URL: nodejs#17134
Fixes: nodejs/node-v8#24
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@snyk-bot snyk-bot mentioned this issue Dec 18, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: implement v8::TaskRunner API in NodePlatform addaleax/node
4 participants
@bmeurer @addaleax @targos @gahaas