From 1a94b859dbde4e002b4b3c04dfedf3ba97804962 Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Mon, 7 Dec 2020 12:52:53 +0100 Subject: [PATCH] fix(runtime): excessive latency introduced by sleep (#2298) When recovering from `EAGAIN`, the `@jsii/runtime` recently added a synchronous sleep for 50 milliseconds. However in large and complex applications, this can add up to a lot of time, as worst case scenario, the sleep can occur before and after each call (once on read, once on write). This change removes the `sleep` until we come up with a better solution. Fixes #2284 --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0 --- .all-contributorsrc | 27 ++++ README.md | 194 ++++++++++++----------- packages/@jsii/runtime/jest.config.ts | 4 + packages/@jsii/runtime/lib/sleep.ts | 14 -- packages/@jsii/runtime/lib/sync-stdio.ts | 10 +- 5 files changed, 133 insertions(+), 116 deletions(-) delete mode 100644 packages/@jsii/runtime/lib/sleep.ts diff --git a/.all-contributorsrc b/.all-contributorsrc index e3e0713a5b..005d41080c 100644 --- a/.all-contributorsrc +++ b/.all-contributorsrc @@ -983,6 +983,33 @@ "bug" ] }, + { + "login": "ThomasSteinbach", + "name": "Thomas Steinbach", + "avatar_url": "https://avatars0.githubusercontent.com/u/1683246?v=4", + "profile": "https://github.com/ThomasSteinbach", + "contributions": [ + "bug" + ] + }, + { + "login": "Ophirr33", + "name": "Ty Coghlan", + "avatar_url": "https://avatars2.githubusercontent.com/u/15920577?v=4", + "profile": "https://ty.coghlan.dev/", + "contributions": [ + "bug" + ] + }, + { + "login": "slotnick", + "name": "Dave Slotnick", + "avatar_url": "https://avatars3.githubusercontent.com/u/918175?v=4", + "profile": "https://github.com/slotnick", + "contributions": [ + "bug" + ] + }, { "login": "majasb", "name": "Maja S Bratseth", diff --git a/README.md b/README.md index b47a36d42c..c7d8b7cf3f 100644 --- a/README.md +++ b/README.md @@ -366,131 +366,135 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - + + + + + +

Abdallah Hodieb

πŸ›

ajnarang

πŸ€”

Alex Pulver

πŸ›

Andy Slezak

πŸ’»

Anshul Guleria

πŸ€”

Ari Palo

πŸ€”

Hamza Assyad

πŸ› πŸ’» πŸ₯… πŸ€” πŸ‘€

AWS CDK Automation

🚧 πŸ‘€

Aaron Costley

πŸ› πŸ’» πŸ₯… πŸ€” πŸ‘€

Abdallah Hodieb

πŸ›

Adam Ruka

πŸ› πŸ’» 🚧 πŸ‘€

Alex Pulver

πŸ›

Andy Slezak

πŸ’»

Anshul Guleria

πŸ€”

AWS CDK Automation

🚧 πŸ‘€

Ben Walters

πŸ€”

BartΕ‚omiej Jurek

πŸ›

Benjamin Maizels

πŸ’» πŸ‘€

Brecht Verhoeve

πŸ€”

CaerusKaru

πŸ’» 🚧

Campion Fellin

πŸ’» β˜•οΈ

Ari Palo

πŸ€”

BartΕ‚omiej Jurek

πŸ›

Ben Walters

πŸ€”

Benjamin Maizels

πŸ’» πŸ‘€

Bill Cauchois

πŸ€”

Brecht Verhoeve

πŸ€”

Breland Miley

πŸ’»

Carter Van Deuren

πŸ›

Christopher Currie

πŸ’» πŸ€”

Raphael

πŸ›

Aaron Costley

πŸ› πŸ’» πŸ₯… πŸ€” πŸ‘€

CyrusNajmabadi

πŸ› πŸ€”

Dongie Agnir

πŸ’» β˜•οΈ πŸ‘€

Daniel Dinu

πŸ› πŸ’»

CaerusKaru

πŸ’» 🚧

Campion Fellin

πŸ’» β˜•οΈ

Carter Van Deuren

πŸ›

Christopher Currie

πŸ’» πŸ€”

CyrusNajmabadi

πŸ› πŸ€”

Daniel Dinu

πŸ› πŸ’»

Daniel Schroeder

πŸ› πŸ’» πŸ“– πŸ€” 🚧

dependabot-preview[bot]

πŸ› 🚧

dependabot[bot]

🚧

dheffx

πŸ›

Shane Witbeck

πŸ€”

Donald Stufft

πŸ› πŸ’» πŸ€” 🐍 πŸ‘€

Junix

πŸ›

Elad Ben-Israel

πŸ› πŸ’» πŸ€” β˜•οΈ 🚧 πŸ‘€ πŸ“’

Dave Slotnick

πŸ›

Donald Stufft

πŸ› πŸ’» πŸ€” 🐍 πŸ‘€

Dongie Agnir

πŸ’» β˜•οΈ πŸ‘€

Eduardo Sena S. Rosa

πŸ›

Elad Ben-Israel

πŸ› πŸ’» πŸ€” β˜•οΈ 🚧 πŸ‘€ πŸ“’

Eli Polonsky

πŸ› πŸ’» πŸ€” 🚧 πŸ‘€

Eric Z. Beard

πŸ“†

Eric Z. Beard

πŸ“†

Fabio Gentile

πŸ›

James Mead

πŸ’»

Jason Fulghum

πŸ€” πŸ“† πŸ‘€

Mitch Garnaat

πŸ› πŸ’» πŸ€” 🐍 πŸ‘€

The Gitter Badger

πŸ’» 🚧

Graham Lea

πŸ€” πŸ‘€

Erik Karlsson

πŸ›

Eugene Kozlov

πŸ’»

Fabio Gentile

πŸ›

Florian Eitel

πŸ€”

Graham Lea

πŸ€” πŸ‘€

Hamza Assyad

πŸ› πŸ’» πŸ₯… πŸ€” πŸ‘€

Hari Pachuveetil

πŸ“

gregswdl

πŸ›

Thorsten Hoeger

πŸ’»

Eli Polonsky

πŸ› πŸ’» πŸ€” 🚧 πŸ‘€

James Siri

πŸ’» 🚧

Jason Del Ponte

πŸšΆβ€β™€οΈ πŸ€” πŸ‘€

Jerry Kindall

πŸ“– πŸ€”

Joseph Lawson

πŸ‘€

Hsing-Hui Hsu

πŸ’» πŸ“– πŸšΆβ€β™€οΈ πŸ€” πŸ‘€

James Mead

πŸ’»

James Siri

πŸ’» 🚧

Jason Del Ponte

πŸšΆβ€β™€οΈ πŸ€” πŸ‘€

Jason Fulghum

πŸ€” πŸ“† πŸ‘€

Jerry Kindall

πŸ“– πŸ€”

Jimmy Gaussen

πŸ€”

Joseph Martin

πŸ›

Justin Taylor

πŸ›

Jon Steinich

πŸ› πŸ€”

Quentin Loos

πŸ€”

Kyle Thomson

β˜•οΈ πŸ‘€

Eugene Kozlov

πŸ’»

Vladimir Shchur

πŸ›

Jon Steinich

πŸ› πŸ€”

Joseph Lawson

πŸ‘€

Joseph Martin

πŸ›

Junix

πŸ›

Justin Taylor

πŸ›

Kyle Thomson

β˜•οΈ πŸ‘€

Leandro Padua

πŸ›

Leandro Padua

πŸ›

Marcos Diez

πŸ›

Matthew Bonig

πŸ› πŸ“

Erik Karlsson

πŸ›

mergify[bot]

🚧

Mike Lane

πŸ›

Breland Miley

πŸ’»

Maja S Bratseth

πŸ›

Marcos Diez

πŸ›

Matthew Bonig

πŸ› πŸ“

Matthew Pirocchi

πŸ’» πŸ₯… πŸ€” πŸ‘€

Mike Lane

πŸ›

Mitch Garnaat

πŸ› πŸ’» πŸ€” 🐍 πŸ‘€

Mitchell Valine

πŸ› πŸ’» πŸšΆβ€β™€οΈ πŸ€” 🚧 πŸ‘€

Matthew Pirocchi

πŸ’» πŸ₯… πŸ€” πŸ‘€

Mitchell Valine

πŸ› πŸ’» πŸšΆβ€β™€οΈ πŸ€” 🚧 πŸ‘€

Neta Nir

πŸ’» πŸ€” 🚧 πŸ‘€

Noah Litov

πŸ’» 🚧 πŸ‘€

Niranjan Jayakar

πŸ› πŸ’» πŸ€” 🚧 πŸ‘€

Nick Lynch

πŸ› πŸ’» 🚧 πŸ‘€

Jimmy Gaussen

πŸ€”

Neta Nir

πŸ’» πŸ€” 🚧 πŸ‘€

Nick Lynch

πŸ› πŸ’» 🚧 πŸ‘€

Niranjan Jayakar

πŸ› πŸ’» πŸ€” 🚧 πŸ‘€

Noah Litov

πŸ’» 🚧 πŸ‘€

PIDZ - Bart

πŸ€”

Petra Barus

πŸ’»

Philip Cali

πŸ€”

Petra Barus

πŸ’»

Philip Cali

πŸ€”

PIDZ - Bart

πŸ€”

Richard H Boyd

πŸ›

Rico Huijbers

πŸ› πŸ’» πŸ€” 🚧 🐍 πŸ‘€

Romain Marcadier

πŸ› πŸ’» 🎨 πŸ₯… πŸšΆβ€β™€οΈ πŸ€” β˜•οΈ 🚧 🐍 πŸ‘€

SADIK KUZU

πŸ‘€

Quentin Loos

πŸ€”

Raphael

πŸ›

Richard H Boyd

πŸ›

Rico Huijbers

πŸ› πŸ’» πŸ€” 🚧 🐍 πŸ‘€

Romain Marcadier

πŸ› πŸ’» 🎨 πŸ₯… πŸšΆβ€β™€οΈ πŸ€” β˜•οΈ 🚧 🐍 πŸ‘€

SADIK KUZU

πŸ‘€

SK

πŸ€”

Sam Goodwin

πŸ‘€

seiyashima42

πŸ› πŸ’» πŸ“–

Tim Wagner

πŸ› πŸ€”

Shiv Lakshminarayan

πŸ’» 🚧 πŸ‘€

SK

πŸ€”

Adam Ruka

πŸ› πŸ’» 🚧 πŸ‘€

Sebastian Korfmann

πŸ› πŸ’» πŸ€”

Sam Fink

πŸ’» πŸ‘€

Sam Goodwin

πŸ‘€

Sebastian Korfmann

πŸ› πŸ’» πŸ€”

Shane Witbeck

πŸ€”

Shiv Lakshminarayan

πŸ’» 🚧 πŸ‘€

Somaya

πŸ’» πŸ€” 🚧 πŸ‘€

The Gitter Badger

πŸ’» 🚧

Hsing-Hui Hsu

πŸ’» πŸ“– πŸšΆβ€β™€οΈ πŸ€” πŸ‘€

Somaya

πŸ’» πŸ€” 🚧 πŸ‘€

Sam Fink

πŸ’» πŸ‘€

sullis

πŸ’»

Thomas Poignant

πŸ›

Tobias Lidskog

πŸ’»

Tyler van Hensbergen

πŸ€”

Thomas Poignant

πŸ›

Thomas Steinbach

πŸ›

Thorsten Hoeger

πŸ’»

Tim Wagner

πŸ› πŸ€”

Tobias Lidskog

πŸ’»

Ty Coghlan

πŸ›

Tyler van Hensbergen

πŸ€”

Daniel Schroeder

πŸ› πŸ’» πŸ“– πŸ€” 🚧

vaneek

πŸ›

Vlad Hrybok

πŸ›

Bill Cauchois

πŸ€”

Florian Eitel

πŸ€”

Yan Zhulanow

πŸ’»

Eduardo Sena S. Rosa

πŸ›

Vlad Hrybok

πŸ›

Vladimir Shchur

πŸ›

Yan Zhulanow

πŸ’»

ajnarang

πŸ€”

deccy-mcc

πŸ›

dependabot-preview[bot]

πŸ› 🚧

dependabot[bot]

🚧

Hari Pachuveetil

πŸ“

deccy-mcc

πŸ›

Maja S Bratseth

πŸ›

dheffx

πŸ›

gregswdl

πŸ›

mergify[bot]

🚧

seiyashima42

πŸ› πŸ’» πŸ“–

sullis

πŸ’»

vaneek

πŸ›
- + + This project follows the [all-contributors](https://github.com/all-contributors/all-contributors) specification. diff --git a/packages/@jsii/runtime/jest.config.ts b/packages/@jsii/runtime/jest.config.ts index 8663425af0..84550ec62b 100644 --- a/packages/@jsii/runtime/jest.config.ts +++ b/packages/@jsii/runtime/jest.config.ts @@ -1,6 +1,10 @@ import { overriddenConfig } from '../../../jest.config'; export default overriddenConfig({ + coveragePathIgnorePatterns: [ + // This cannot be tested in a way that enables collection of coverage + '/lib/sync-stdio.ts', + ], coverageThreshold: { global: { branches: 48, diff --git a/packages/@jsii/runtime/lib/sleep.ts b/packages/@jsii/runtime/lib/sleep.ts deleted file mode 100644 index 002c23cd18..0000000000 --- a/packages/@jsii/runtime/lib/sleep.ts +++ /dev/null @@ -1,14 +0,0 @@ -// We need a shared buffer array for the purpose of this exercise. -const array = new Int32Array(new SharedArrayBuffer(4)); - -/** - * Sleeps for a set amount of time, synchronously. - * - * @param time the amount of time to wait, in milliseconds. - */ -export function sleep(time: number): void { - // `Atomics.wait` will block for `time` milliseconds if `array[0]` still has - // value `0` (which it will, since we just initialized it to that). The return - // value is irrelevant for our business here. - Atomics.wait(array, 0, 0, time); -} diff --git a/packages/@jsii/runtime/lib/sync-stdio.ts b/packages/@jsii/runtime/lib/sync-stdio.ts index e3a57e386a..053e22f382 100644 --- a/packages/@jsii/runtime/lib/sync-stdio.ts +++ b/packages/@jsii/runtime/lib/sync-stdio.ts @@ -1,7 +1,5 @@ import * as fs from 'fs'; -import { sleep } from './sleep'; - // Note: the `process.std{in,out,err}.fd` is not part of the `@types/node` declarations, because // those cannot model how those fields are guaranteed to be absent within the context of worker // threads. The should be present here, but since we must resort to `as any`, we take the extra @@ -53,7 +51,8 @@ export class SyncStdio { if (e.code !== 'EAGAIN') { throw e; } - sleep(50 /*ms*/); + // We'll retry immediately, and possibly thrash the CPU until the buffer was drained. We + // do not currently have a better way around. } } } @@ -96,10 +95,7 @@ function readSync( // attempts, sleeping too much would slow everything to a crawl, and not enough would cause // significant wasting of CPU cycles. case 'EAGAIN': - // Keep trying until it no longer says EAGAIN. We'll be waiting a little before retrying - // in order to avoid thrashing the CPU like there is no tomorrow. This is not entirely - // ideal, but it has to do. - sleep(50 /*ms*/); + // Thrashing the CPU as previously discussed... break; // HACK: in Windows, when STDIN (aka FD#0) is wired to a socket (as is the case when started