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.

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

Start to add rust backed request execution under a feature-flag by rewriting ConfigRequest #9658

Merged
merged 32 commits into from May 1, 2024

Conversation

yamadapc
Copy link
Contributor

@yamadapc yamadapc commented Apr 19, 2024

Does not achieve anything in particular other than set patterns for exposing/integrating rust code under a flag.

While implementing unit-tests for the ConfigRequest I found a bug where invalidateOnEnvChange would not be picked-up on its own.

There are exhaustive tests for the config request current/previous implementations added.

For the hashing section, there is a slight missing part regarding sorting of object keys.

@yamadapc yamadapc requested a review from a team April 19, 2024 04:19
@yamadapc yamadapc marked this pull request as draft April 19, 2024 04:19
@yamadapc yamadapc changed the title Start to add rust backed request execution under a feature-flag by rewriting ConfigRequest WIP - Start to add rust backed request execution under a feature-flag by rewriting ConfigRequest Apr 19, 2024
.mocharc.json Show resolved Hide resolved
@yamadapc yamadapc changed the title WIP - Start to add rust backed request execution under a feature-flag by rewriting ConfigRequest Start to add rust backed request execution under a feature-flag by rewriting ConfigRequest Apr 22, 2024
@yamadapc yamadapc marked this pull request as ready for review April 22, 2024 04:48
@devongovett
Copy link
Member

I actually want to remove ConfigRequest completely. It's not a real request at the moment actually since it doesn't do any work. The configs have actually already been loaded, and the ConfigRequest is fake just to setup the request graph structure. In reality, it isn't necessary at all - the invalidations could just be added to the parent request. The config loading doesn't get invalidated separately right now anyway.

In addition, it looks like there's a lot of bridging here. Since ConfigRequest doesn't actually do any work other than mutating the graph, now we're calling into rust only to call back into JS several times. Maybe ConfigRequest was just an example of the setup but perhaps it would be more efficient to do the request work in rust and send back all the invalidations at once as part of the result rather than calling back to JS via the RequestApi a whole bunch of times?

@yamadapc
Copy link
Contributor Author

That makes sense; the purpose of this PR is:

  • Start migrating
  • Show JS delegation / interop patterns we can use
  • Write tests

Maybe ConfigRequest was just an example of the setup but perhaps it would be more efficient to do the request work in rust and send back all the invalidations at once as part of the result rather than calling back to JS via the RequestApi a whole bunch of times?

Yes this is just an example so it doesn't do anything interesting.

That said, I feel strongly we should not optimise anything without measuring. The benchmarks added in https://github.com/yamadapc/napi-benchmark/tree/main show there's very low overhead to call JavaScript functions from rust (nanosecond cost for calls in of themselves).

Optimising without benchmarks or measurements has a high risk of implementing something that is at the same time hard to understand and that potentially isn't faster than a naive implementation.

@yamadapc yamadapc requested a review from marcins April 22, 2024 07:12
@alshdavid
Copy link
Contributor

What problem would be solved by locking a Rust thread but calling into JS from a background thread?

Essentially, for every 1 JavaScript execution context/worker, napi demands 1 dedicated Rust thread. You cannot pass JavaScript references between threads (with some exceptions) so data is typically handled through channels. If you want to do non blocking JS behaviours, that must be factored into the way you write the napi bindings and is quite verbose.

@yamadapc yamadapc enabled auto-merge (squash) April 30, 2024 02:43
@yamadapc yamadapc disabled auto-merge April 30, 2024 04:51
@yamadapc yamadapc enabled auto-merge (squash) April 30, 2024 23:12
auto-merge was automatically disabled April 30, 2024 23:13

Merge queue setting changed

@yamadapc yamadapc enabled auto-merge April 30, 2024 23:13
@yamadapc yamadapc added this pull request to the merge queue May 1, 2024
@yamadapc yamadapc removed this pull request from the merge queue due to a manual request May 1, 2024
@yamadapc yamadapc enabled auto-merge May 1, 2024 02:00
@yamadapc yamadapc added this pull request to the merge queue May 1, 2024
auto-merge was automatically disabled May 1, 2024 02:28

Merge queue setting changed

@devongovett devongovett removed this pull request from the merge queue due to the queue being cleared May 1, 2024
@mattcompiles mattcompiles merged commit 20c4a9b into parcel-bundler:v2 May 1, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants