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

Add SharedArrayBuffer support #46

Merged
merged 10 commits into from
Apr 10, 2023
Merged

Add SharedArrayBuffer support #46

merged 10 commits into from
Apr 10, 2023

Conversation

jdesboeufs
Copy link
Contributor

@jdesboeufs jdesboeufs commented Mar 26, 2023

Hi @mourner,

I hope you, your family and friends are still safe.

This PR adds support for SharedArrayBuffer which is a fully compatible alternative to ArrayBuffer.

Firstly it allows a SharedArrayBuffer to be given to Flatbush.from(). Up to now it throws Data must be an instance of ArrayBuffer.

Secondly it adds a new optional parameter to the constructor: useSharedArrayWorker.

Before to go further, I need to know what do you think of this.

Why use a SharedArrayBuffer? You can share the data between multiple workers (Worker, SharedWorker, ServiceWorker and lower memory footprint.
In my use case, the R-tree is big! (~2.5GB).

Jerome

Copy link
Owner

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Overall this definitely makes sense, thanks for the PR! Left a few nits inline. Also, I'm wondering if this could be covered by some kind of tests, even if SharedArrayBuffer is mocked?

index.js Outdated
@@ -11,8 +11,8 @@ const VERSION = 3; // serialized format version
export default class Flatbush {

static from(data) {
if (!(data instanceof ArrayBuffer)) {
throw new Error('Data must be an instance of ArrayBuffer.');
if (!(data instanceof ArrayBuffer) && SharedArrayBuffer && !(data instanceof SharedArrayBuffer)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Two issues here, if I'm following this right:

  • In case SharedArrayBuffer isn't available, just referencing it will throw an error, so this won't work as a check. We need to use typeof.
  • If we fix the reference, the whole condition will still evaluate to false in case of a wrong data type, so the error won't be thrown.

So this would need to look something like this:

Suggested change
if (!(data instanceof ArrayBuffer) && SharedArrayBuffer && !(data instanceof SharedArrayBuffer)) {
if (!(data instanceof ArrayBuffer) && (typeof SharedArrayBuffer === 'undefined' || !(data instanceof SharedArrayBuffer))) {

index.js Outdated
if (numItems === undefined) throw new Error('Missing required argument: numItems.');
if (isNaN(numItems) || numItems <= 0) throw new Error(`Unpexpected numItems value: ${numItems}.`);
if (isNaN(numItems) || numItems <= 0) throw new Error(`Unexpected numItems value: ${numItems}.`);
if (useSharedArrayBuffer && !SharedArrayBuffer) throw new Error('SharedArrayBuffer not available in your runtime.');
Copy link
Owner

Choose a reason for hiding this comment

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

Same issue, typeof

@jdesboeufs
Copy link
Contributor Author

I finally used global.SharedArrayBuffer to avoid the error and to allow unit testing.

@mourner
Copy link
Owner

mourner commented Apr 8, 2023

@jdesboeufs global. only works in Node 😅 We could use globalThis, although it's also a relatively recent API. I think the typeof undefined check is the safest option. You could do it once in the top scope and save in a variable.

@jdesboeufs
Copy link
Contributor Author

I'm definitely a backend developer 🤣

I agree with you for typeof but it make it difficult to test SharedArrayBuffer absence without some kind of dependency injection…

Here is another implementation, using data.constructor.name to detect the object type, and which replace useSharedArrayBuffer by ArrayBufferType (default ArrayBuffer). More consistent with other arguments, less code, and let user inject SharedArrayBuffer by itself (no need to check availability).

As ArrayBuffer and SharedArrayBuffer are native and relatively recent things in JS world, we can consider that obj.constructor.name is OK. If somebody try to create an index from an object without constructor it will have a generic error.

Tell me what to you think! 😅

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@jdesboeufs
Copy link
Contributor Author

I agree with that!

If it's OK feel free to squash. No need to bloat the commit history 🫣

Copy link
Owner

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the contribution.

@mourner mourner merged commit 3e82ac6 into mourner:main Apr 10, 2023
@jdesboeufs jdesboeufs deleted the shared-array-buffer branch April 10, 2023 12:45
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.

2 participants