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

Support solana-web3.js batching #23628

Closed
wants to merge 8 commits into from
Closed

Conversation

epicfaace
Copy link
Contributor

@epicfaace epicfaace commented Mar 13, 2022

Summary of Changes

Fixes #23627 by adding a config option autoBatch to Connection that, if set to true, batches all JSON RPC requests so they are sent every 50ms.

@mergify mergify bot added the community Community contribution label Mar 13, 2022
@mergify mergify bot requested a review from a team March 13, 2022 00:07
@epicfaace epicfaace changed the title Support solana-web3.js automatic batching Support solana-web3.js batching Mar 13, 2022
@epicfaace epicfaace marked this pull request as ready for review March 13, 2022 04:07
@t-nelson
Copy link
Contributor

we can't break API like this. i don't think we want this to be the default behavior anyway (imposed batching delay). can you make it a separate function?

@epicfaace
Copy link
Contributor Author

we can't break API like this. i don't think we want this to be the default behavior anyway (imposed batching delay). can you make it a separate function?

Hi @t-nelson -- this isn't a breaking change, the default behavior is still the same as before. The batching behavior is only triggered when you pass in autoBatch: true to the connection options. Does that work?

@stale
Copy link

stale bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 16, 2022
@epicfaace
Copy link
Contributor Author

this isn't a breaking change, the default behavior is still the same as before. The batching behavior is only triggered when you pass in autoBatch: true to the connection options. Does that work?

Hi @t-nelson -- see my comment above. Does this work for you? Let me know if you'd like me to make any additional changes.

@stale stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 17, 2022
Copy link
Contributor

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! I like batching, in principle. If we do it, we should do it with a global lock that people can opt into in userspace.

For a long time, this is how React itself performed batching.

React.unstable_batchedUpdates(() => {
  setSomeState(true);
  callSomeMethodThatMaybeSetsSomeState(123);
  if (something) {
    setSomething(something);
  }
});

Essentially, the implementation was something like:

function unstable_batchedUpdates(cb) {
  _batch = true;
  try {
    cb();
  } finally {
    _batch = false;
  }
}

…and that _batch variable was accessible by whatever needed to make the batch-or-not-to-batch decision.

Ours might look something like this:

connection.batchRequests(() => {
  connection.requestAirdrop();
  methodThatMightMakeARequest();
  if (needBlockTime) {
    connection.getBlockTime();
  }
});

From there, there would be a ton of work to actually make that work.

Things that would need to be resolved:

  • Right now _rpcBatchRequest is written in such a way that one error nukes the entire batch. It doesn't understand that the return value of a batch is an array of the successes and and an array of the errors. If even one request in a batch errors, it nukes the whole promise: https://github.com/steveluscher/solana/blob/5eae7b708c45979cc3573e6be2b932e87bd9ec38/web3.js/src/connection.ts#L1061-L1067. That will need to be fixed. Mental note for me that this sort of means getParsedTransactions is broken, for some definition of broken.
  • The API I've suggested above is really confusing if you're used to request method being async. How would you write the above if methodThatMightMakeARequest depended on requestAirdrop finishing first? Is that two batches now? Is it even possible to build a dependency tree of which calls depend on which results, so that you can prepare exactly the number of batches necessary?

@stale
Copy link

stale bot commented Apr 30, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 30, 2022
@epicfaace
Copy link
Contributor Author

Still working on this

@stale stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 30, 2022
@epicfaace
Copy link
Contributor Author

If we do it, we should do it with a global lock that people can opt into in userspace.

@steveluscher thanks for the feedback. Isn't that (global lock people can opt into in userspace) what this PR enables? Or did you have something else in mind?

@steveluscher
Copy link
Contributor

This PR can be implemented as a customFetch on Connection! You should repurpose it as such, and open source it.

More detail, here: #23627 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

solana-web3.js: Automatically batch requests made via JSON RPC
3 participants