-
Notifications
You must be signed in to change notification settings - Fork 15
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
Sorting an ArrayOfBufferBackedObjects doesn't actually work? #18
Comments
Interesting. Yeah that is possible. Do you mind writing a test/example that is a lot more reduced (for example without Comlink). The |
I was trying to make a pull request but it wants me to sign something. Here you go. it("can be sorted", function () {
// Copyright: user ibsusu on github.com
// License: I don't care what you do with the code below. You have all rights to the below code and the rights will never be revoked by ibsusu or anyone else.
// I'm not signing the contributor license agreement.
// I just want sorting to work.
const descriptors = {
firstKey: BufferBackedObject.UTF8String(80),
middleKey: BufferBackedObject.ArrayBuffer(10),
};
const descriptorSize = structSize(descriptors);
const randomNames = [
"bill",
"mike",
"dave",
"kat",
"brian",
"adam",
"sammy",
"martin",
"yanaris",
];
// the expected order of the names
const firstKeyOrderedNames = [
"adam",
"bill",
"brian",
"dave",
"kat",
"martin",
"mike",
"sammy",
"yanaris",
];
const buffer = new ArrayBuffer(descriptorSize * randomNames.length);
const boar = new ArrayOfBufferBackedObjects(buffer, descriptors);
// this may be unnecessary for testing
let randomNumberArrays = [
[61, 129, 35, 3, 65, 151, 190, 139, 89, 212],
[43, 196, 83, 213, 118, 216, 142, 132, 220, 85],
[223, 245, 232, 30, 114, 42, 254, 129, 112, 59],
[159, 73, 73, 38, 199, 212, 60, 100, 181, 248],
[22, 197, 38, 37, 46, 139, 77, 202, 181, 183], // dupe
[178, 69, 33, 61, 59, 53, 116, 117, 66, 81],
[232, 97, 56, 20, 140, 122, 162, 88, 242, 58],
[24, 147, 73, 201, 205, 253, 200, 191, 99, 183],
[22, 197, 38, 37, 46, 139, 77, 202, 181, 183], // dupe
[190, 154, 53, 52, 94, 109, 131, 101, 43, 172],
[168, 220, 117, 172, 64, 219, 129, 80, 207, 43],
];
// fill in the BBOs
for (let i = 0; i < randomNames.length; ++i) {
// fill the strings
const bbo = boar[i];
bbo.firstKey = randomNames[i];
// fill the arraybuffers
// let dataView = new DataView(bbo.middleKey);
// for(let j=0;j<10;++j){
// dataView.setUint8(j, randomNumberArrays[i][j]);
// }
}
// our compare function
function compareFirst(itemA, itemB) {
if (itemA.firstKey < itemB.firstKey) {
return -1;
}
if (itemA.firstKey > itemB.firstKey) {
return 1;
}
return 0;
}
// sort it and get the list of names
boar.sort(compareFirst);
let sortedNames = boar.map((bbo) => { return bbo.firstKey;});
console.log("sortedNames", sortedNames);
// check that the boar is sorted via the normal 'get' routine
for (let i = 0; i < sortedNames; ++i) {
expect(sortedNames[i]).toBe(firstKeyOrderedNames[i]);
}
// check that the boar is sorted via pulling directly from the buffer.
for (let i = 0; i < boar.length; ++i) {
let nameInBuffer = new TextDecoder()
.decode(new Uint8Array(boar.buffer, i * descriptorSize, 80))
.replace(/\u0000+$/, "");
console.log("name in buffer", nameInBuffer);
expect(nameInBuffer).toBe(firstKeyOrderedNames[i]);
}
}); |
Thanks for that! This was really helpful. So you are right. (cc @developit in case he has any ideas) |
One option would be to instead provide a Set-compatible API rather than an Array lookalike. Arrays are tricky to proxy with support for push(), splice(), .length=n, etc. It's also interesting to consider the performance implications and potential that a Set derivative would have. Removals could be implemented using copyWithin, and has() could search only at each byte offset within the backing buffer to compare the buffers directly. One thing I ran into with someone who was using the AOBBO setup from this library was that assignment of whole objects to an Array index doesn't work (which is reasonable, but the Array API surface makes it less obvious that this will be the case). |
got assignment working. check this gist for the line. The funny thing is that using TimSort on this doesn't work. I think the reason is that assignment of the BBO to a tmp variable is essentially just creating a reference to that location in the ArrayBuffer rather than a copy of the Uint8Array and the underlying buffer. I'm not sure how to handled this tbh. |
Got sorting working. The implementation is a bit different from the original so just pull what you need from it or use it all. It passes all of the original tests + the sorting test. it utilizes a custom sort but any(?) sort will work so long as any pivot or swap value is modified to use a separate buffer. https://gist.github.com/ibsusu/cc95a8f7aed4a1d58b54c1db256bcb79 I'm done, going to sleep. |
Alright, last iteration since I have a passable implementation working in my development code now. There are some things to note regarding functionality:
all the files + some simple benchmarking Just some thoughts here:
let descriptors = {
name: Bbo.UTF8String(90),
otherKey: Bbo.UTF8String(10)
}; took 0.1235 seconds with a simple comparison function on the name key. Sorting 100,000 AOBBOs on the originating thread using a comparison function and quicksort took 27.09 seconds, and sorting 100,000 AOBBOs on a webworker using the direct ArrayBuffer accessing TextEncoded string comparison in quicksort on the webworker took 4.3975 seconds so it's still pretty slow. Anyways I'm going to move on in my project now. Thanks for creating this library, it's really interesting. Feel free to use some or none of the code. ✌️ |
Should I create my own sort function in order to make this work? The built in ArrayOfBufferBackedObjects sort doesn't actually sort the buffer.
The text was updated successfully, but these errors were encountered: