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

Module Seperation, Signed WasmMemory views, and minor fixes #56

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from

Conversation

JohnDog3112
Copy link
Collaborator

Module Separation: #22

  • As suggested in the linked issue, I added an ID to both IWasmModule and IWasmModuleAsync that use a shared getNextModuleID function so every module has their own unique ID.
  • I used this ID to separate precomputed objects in twrconcanvas between modules. This was done via combining the precomputed object ID and the module ID into a 52 bit key (22 bits for module ID, 32 for object ID) to index the precomputed objects list.
  • I did something similar for the audio node and playback lists in the audio library.

Signed WasmMemory views: #52

  • Added signed views for 8, 16, and 32 bit integers in WasmMemory
  • Renamed the previous unsigned views to mem8u, mem16u, and mem32u respectively with the signed views taking the old names
  • Used the signed views in the audio library rather than converting it to signed by hand
  • TODO: Ensure the documentation is up to date

Minor Fixes:

  • Removed canvas from tests-audio that was copied over during setup
  • Fixed initial problem with single player pong unnecessary scrolling due to extra width #38 using the fact that the deeplinks in pong cause a refresh allowing the canvas to be resized before initializing the wasm module. Linked issue should probably be renamed or have the later stuff be split into it's own issue so it can be closed.
  • Fixed an issue where PutImageData was incompatible with getImageData while using an AsyncModule

Copy link
Owner

@twiddlingbits twiddlingbits left a comment

Choose a reason for hiding this comment

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

Instead of a global function (getNextModuleID), you could use a static member variable and function, which would be a little cleaner? For an example, search for "static unqiueInt:number=1;" in class twrEventQueueReceive

Copy link
Owner

@twiddlingbits twiddlingbits left a comment

Choose a reason for hiding this comment

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

 //should be equivalent to (mod.id << 32) | id
   return (mod.id & (2**20 - 1)) * 2**32 + id;

Why didn't you just use mod.id << 32) | id ?

@JohnDog3112
Copy link
Collaborator Author

JohnDog3112 commented Oct 31, 2024

 //should be equivalent to (mod.id << 32) | id
   return (mod.id & (2**20 - 1)) * 2**32 + id;

Why didn't you just use mod.id << 32) | id ?

When using a number (rather than BigInt), the left shift operator truncates the number down to 32-bits. Though, it might be better to just use BigInt to ensure that there aren't any floating point errors in the math. Also, I already checked that mod.id could fit into 20 bits so it should probably be reduced to mod.id * 2**32 + id

Copy link
Owner

@twiddlingbits twiddlingbits left a comment

Choose a reason for hiding this comment

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

d2d_idexists

don't forget to add to docs.

Also d2d_doesidexist (or d2d_isidexisting) might be a slightly better name.

@twiddlingbits
Copy link
Owner

When using a number (rather than BigInt), the left shift operator truncates the number down to 32-bits.

Ah, i didn't realize that.

@JohnDog3112
Copy link
Collaborator Author

JohnDog3112 commented Oct 31, 2024

Instead of a global function (getNextModuleID), you could use a static member variable and function, which would be a little cleaner? For an example, search for "static unqiueInt:number=1;" in class twrEventQueueReceive

Where would I put the static reference? I can't find any shared parents between twrWasmModuleAsync and twrWasmModule. The closest is twrWasmBase but for the async module, it's only applied to twrWAsmModuleAsyncProxy which is inaccessible to the libraries. So I'm unsure how to set it up so they can share the same static variable.

However, if they were to be done separately, I could create a static id for each of the modules and then include whether the module is async in the key like so:

function computeID(mod: IWasmModule|IWasmModuleAsync, baseID: number) {
  const isAsync = mod.isAsync ? 1 : 0;
  //insert bounds checks for module ID length and baseID length

  //1 bit for async, 21 bits for module id, 32 bits for baseID
  // (isAsync << 51) | (mod.id << 32) | baseID
  return (isAsync * 2**51) + (mod.id * 2**32) + baseID;
}

@JohnDog3112
Copy link
Collaborator Author

d2d_idexists

don't forget to add to docs.

Also d2d_doesidexist (or d2d_isidexisting) might be a slightly better name.

I prefer d2d_doesidexist, so I'll rename them and add them to the docs.

@JohnDog3112
Copy link
Collaborator Author

I looked over how to do the ID's using statics again and came up with just directly referencing the class they're stored in. So, for now, I have the ID variable under twrWasmBase. In that way, twrWasmModule and twrWasmModuleAsync can access it directly via ++twrWasmBase.uniqueID. Now my only concern is that ID isn't available to twrWasmModuleAsyncProxy, but I'm not sure if that could cause any issues.

@twiddlingbits
Copy link
Owner

I looked over how to do the ID's using statics again and came up with just directly referencing the class they're stored in. So, for now, I have the ID variable under twrWasmBase. In that way, twrWasmModule and twrWasmModuleAsync can access it directly via ++twrWasmBase.uniqueID. Now my only concern is that ID isn't available to twrWasmModuleAsyncProxy, but I'm not sure if that could cause any issues.

I had to look this up to be sure, but perhaps you knew:

In TypeScript (and JavaScript), static variables are associated with the class itself, not with instances of the class. However, each derived class has its own copy of the static variable, independent of the base class and other derived classes.

So i think if you want to have seperate classes (like twrWasmModule, twrWasmModuleAsync) have the same member variable, like id, and you wanted the values to be unique between each other, you could have them each have a static uniqueID, but assign the initial values to a separate range (so twrWasmModule starts at zero, twrWasmModuleAsync starts at MAX/2, for example).

@JohnDog3112
Copy link
Collaborator Author

I knew that's how it worked with interfaces, but I didn't think it did that with class extension. My current implementation is just having the static in twrWasmBase as static uniqueID: number = 0; and then referencing it in twrWasmModule and twrModuleAsync as twrWasmBase.uniqueID. That way they're still unique and get the ID from a (mostly) unique location.

However, is the statics isolated to each module better so that class variables aren't being modified externally? However, would that be better than my previous idea of having their IDs separate and appending a bit in the full ID to specify whether it's async or not as shown above? Both solutions essentially do the same thing, but the function seems more concise to me.

For reference, the other method I talked about it this:

function computeID(mod: IWasmModule|IWasmModuleAsync, baseID: number) {
  const isAsync = mod.isAsync ? 1 : 0;
  //insert bounds checks for module ID length and baseID length

  //1 bit for async, 21 bits for module id, 32 bits for baseID
  // (isAsync << 51) | (mod.id << 32) | baseID
  return (isAsync * 2**51) + (mod.id * 2**32) + baseID;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants