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.

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

readFileSync #452

Merged
merged 3 commits into from Aug 9, 2018

Conversation

3 participants
@ry
Collaborator

ry commented Aug 3, 2018

Depends on #434

@ry ry changed the title from [WIP] readFileSync to readFileSync Aug 7, 2018

@ry ry requested a review from piscisaureus Aug 7, 2018

Show outdated Hide outdated js/assets.ts Outdated
Show outdated Hide outdated js/text_encoding.d.ts Outdated

@ry ry referenced this pull request Aug 7, 2018

Closed

Generate declarations #455

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 8, 2018

Collaborator

@kitsonk I wasn't able to get this to work without including lib.dom.d.ts. It adds 2M to the executable. PTAL.
@piscisaureus PTAL

Collaborator

ry commented Aug 8, 2018

@kitsonk I wasn't able to get this to work without including lib.dom.d.ts. It adds 2M to the executable. PTAL.
@piscisaureus PTAL

Show outdated Hide outdated js/globals.ts Outdated
Show outdated Hide outdated js/globals.ts Outdated
@kitsonk

This comment has been minimized.

Show comment
Hide comment
@kitsonk

kitsonk Aug 9, 2018

Contributor

While this works, we really need to consider how to eliminate lib.dom.d.ts from the runtime, outside of the huge size, it devalues the value of TypeScript. The way global.ts now exists, it doesn't add anything to the global scope, which is "fine" but now Console and console are both coming from lib.dom.d.ts.

For example, given:

console.markTimeline("foo");

Instead of throwing a TypeScript error you get:

$ ./out/debug/deno tests/999_error.ts
TypeError: console.markTimeline is not a function
    at eval (file:///Users/kkelly/github/deno/tests/999_error.ts:1:9)
    at eval (<anonymous>)
    at execute (../../../../js/runtime.ts:219:3)
    at FileModule.compileAndRun (../../../../js/runtime.ts:114:5)
    at denoMain (../../../../js/main.ts:56:7)
    at deno_main.js:1:1

Where on master you get (after clearing your cache):

$ ./out/debug/deno tests/999_error.ts
/Users/kkelly/github/deno/tests/999_error.ts:1:9 - error TS2339: Property 'markTimeline' does not exist on type 'Console'.

1 console.markTimeline("foo");
          ~~~~~~~~~~~~
Contributor

kitsonk commented Aug 9, 2018

While this works, we really need to consider how to eliminate lib.dom.d.ts from the runtime, outside of the huge size, it devalues the value of TypeScript. The way global.ts now exists, it doesn't add anything to the global scope, which is "fine" but now Console and console are both coming from lib.dom.d.ts.

For example, given:

console.markTimeline("foo");

Instead of throwing a TypeScript error you get:

$ ./out/debug/deno tests/999_error.ts
TypeError: console.markTimeline is not a function
    at eval (file:///Users/kkelly/github/deno/tests/999_error.ts:1:9)
    at eval (<anonymous>)
    at execute (../../../../js/runtime.ts:219:3)
    at FileModule.compileAndRun (../../../../js/runtime.ts:114:5)
    at denoMain (../../../../js/main.ts:56:7)
    at deno_main.js:1:1

Where on master you get (after clearing your cache):

$ ./out/debug/deno tests/999_error.ts
/Users/kkelly/github/deno/tests/999_error.ts:1:9 - error TS2339: Property 'markTimeline' does not exist on type 'Console'.

1 console.markTimeline("foo");
          ~~~~~~~~~~~~
@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 9, 2018

Collaborator

This PR is getting too big, I've split it into #488 for the preliminary changes.
I will rebase this on top of #488 once it lands with only the readFileSync stuff.

Collaborator

ry commented Aug 9, 2018

This PR is getting too big, I've split it into #488 for the preliminary changes.
I will rebase this on top of #488 once it lands with only the readFileSync stuff.

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 9, 2018

Collaborator

@piscisaureus PTAL. This one is good to go now. Note the commit message in "Add TextEncoder/TextDecoder support."

@kitsonk Ok, I agree. I changed it so that this no longer relies on lib.dom.d.ts

#434 should land first.

Collaborator

ry commented Aug 9, 2018

@piscisaureus PTAL. This one is good to go now. Note the commit message in "Add TextEncoder/TextDecoder support."

@kitsonk Ok, I agree. I changed it so that this no longer relies on lib.dom.d.ts

#434 should land first.

@@ -14,5 +14,6 @@ void handle_code_cache(Deno* d, uint32_t cmd_id, const char* filename,
void handle_timer_start(Deno* d, uint32_t cmd_id, uint32_t timer_id,
bool interval, uint32_t delay);
void handle_timer_clear(Deno* d, uint32_t cmd_id, uint32_t timer_id);
void handle_read_file_sync(Deno* d, uint32_t cmd_id, const char* filename);

This comment has been minimized.

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

I guess it is appropriate to call the method read_file_sync since there currently is no way to use this function asynchronously.

But we should really try to avoid having two implementations (sync and async) of every I/O function on the privileged side.

The JS layer can manage internally how it's going to present the result to the user; it can either block until recv() is called and return the value (sync), --or-- return nothing and resolve a promise when recv() gets called (async).

I could live with there being an is_sync field in the message, more as a hint to the backend, so that in the simple-sync model it can do I/O on the main thread and instead of offloading to a thread pool.

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

I guess it is appropriate to call the method read_file_sync since there currently is no way to use this function asynchronously.

But we should really try to avoid having two implementations (sync and async) of every I/O function on the privileged side.

The JS layer can manage internally how it's going to present the result to the user; it can either block until recv() is called and return the value (sync), --or-- return nothing and resolve a promise when recv() gets called (async).

I could live with there being an is_sync field in the message, more as a hint to the backend, so that in the simple-sync model it can do I/O on the main thread and instead of offloading to a thread pool.

This comment has been minimized.

@ry

ry Aug 9, 2018

Collaborator

I generally agree with having all the handlers be async and letting the JS layer choose... Two notes tho

  1. we're bootstrapping here - so I'm being quick and dirty with how these things are implemented - with the idea that we'll go back over them again in the near future (like after #462 lands)
  2. we've experienced in node where dispatching to thread pool for fs ops can be much slower than just synchronously doing those ops. I am worried that we may introduce the same here. Tho I suppose that was mostly caused by libuv's use of eventfd?
@ry

ry Aug 9, 2018

Collaborator

I generally agree with having all the handlers be async and letting the JS layer choose... Two notes tho

  1. we're bootstrapping here - so I'm being quick and dirty with how these things are implemented - with the idea that we'll go back over them again in the near future (like after #462 lands)
  2. we've experienced in node where dispatching to thread pool for fs ops can be much slower than just synchronously doing those ops. I am worried that we may introduce the same here. Tho I suppose that was mostly caused by libuv's use of eventfd?

This comment has been minimized.

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

@ry

  1. On bootstrapping: I agree, I'm not saying you should can it. I was just trying to communicate the direction we should take after this lands. By looking at just the code iteself, people can't tell whether this is "just bootstrapping" or "the way to do things going forward".
    Maybe a add a comment?
    This situation is not all that different to the extern_version patch the other day.

  2. On thread pool performance: The libuv thread pool wasn't so great indeed, I can't say I don't worry about it at all. But my strong suspicion is that it's bogged down by the way it signals back from the worker to the main thread. Since the main thread blocks on epoll, there's three syscalls between pool work completing and the main thread picking it up (eventfd write, epoll, eventfd read). If you benchmark with a small read() op from a cache-hot file - in itself not a very expensive operation that takes one syscall - those three extra syscalls are going to be noticeable.

    I think we'll fare much better if we use a lightweight primitive like a condition variable or a futex. It was also not that great that we didn't really have a good system to maintain ordering between operations when we need it, which in practice means that multiple thread pool threads can accept work items which cannot run in parallel, so they're now all serializing w.r.t one another.

    Another source of perceived slowness is the relative inflexibility of the thread pool size. I think we can make it auto scale by running an extra low-priority thread which is normally sleeping (it never receives a CPU quantum because there are always higher-priority threads to run), but when it does wake up we take it as a hint that the other threads are now all blocked on I/O.

    I also preemptively agree that my theories are unproven and we'll need to benchmark :)

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

@ry

  1. On bootstrapping: I agree, I'm not saying you should can it. I was just trying to communicate the direction we should take after this lands. By looking at just the code iteself, people can't tell whether this is "just bootstrapping" or "the way to do things going forward".
    Maybe a add a comment?
    This situation is not all that different to the extern_version patch the other day.

  2. On thread pool performance: The libuv thread pool wasn't so great indeed, I can't say I don't worry about it at all. But my strong suspicion is that it's bogged down by the way it signals back from the worker to the main thread. Since the main thread blocks on epoll, there's three syscalls between pool work completing and the main thread picking it up (eventfd write, epoll, eventfd read). If you benchmark with a small read() op from a cache-hot file - in itself not a very expensive operation that takes one syscall - those three extra syscalls are going to be noticeable.

    I think we'll fare much better if we use a lightweight primitive like a condition variable or a futex. It was also not that great that we didn't really have a good system to maintain ordering between operations when we need it, which in practice means that multiple thread pool threads can accept work items which cannot run in parallel, so they're now all serializing w.r.t one another.

    Another source of perceived slowness is the relative inflexibility of the thread pool size. I think we can make it auto scale by running an extra low-priority thread which is normally sleeping (it never receives a CPU quantum because there are always higher-priority threads to run), but when it does wake up we take it as a hint that the other threads are now all blocked on I/O.

    I also preemptively agree that my theories are unproven and we'll need to benchmark :)

This comment has been minimized.

@ry

ry Aug 9, 2018

Collaborator

I also preemptively agree that my theories are unproven and we'll need to benchmark :)

: )

@ry

ry Aug 9, 2018

Collaborator

I also preemptively agree that my theories are unproven and we'll need to benchmark :)

: )

Show outdated Hide outdated src/handlers.rs Outdated
Show outdated Hide outdated js/os.ts Outdated
Show outdated Hide outdated js/text_encoding.ts Outdated

ry added some commits Aug 7, 2018

Add TextEncoder/TextDecoder support.
Fixes #470

This commit increases size:
out/release/gen/bundle/main.js      7.3M -> 7.9M
out/release/gen/bundle/main.js.map   11M -> 12M
out/release/gen/snapshot_deno.bin    34M -> 37M
out/release/deno                     49M -> 53M

Note the amount in the JS code added is quite small:
4.0K    node_modules/text-encoding/index.js
4.0K    node_modules/@types/text-encoding/index.d.ts
4.0K    js/text_encoding.ts

Unclear to me what is causing the jump in snapshot size.

@ry ry merged commit 413bcf2 into master Aug 9, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@ry ry deleted the read_file_sync branch Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment