Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement hard cloning of data and vector #16

Merged
merged 1 commit into from
Nov 28, 2023
Merged

Conversation

kylebarron
Copy link
Member

@kylebarron kylebarron commented Nov 27, 2023

For #8

@kylebarron kylebarron changed the title Implement hard clonig of data and vector Implement hard cloning of data and vector Nov 27, 2023
@kylebarron
Copy link
Member Author

image

this is hard to test because node doesn't have ArrayBuffer.transfer available, but checked manually in chrome and, originally the geometryData was shared (loaded from IPC) but after hardClone, isShared accurately said it was no longer shared, and even when I transfer one of the original buffers, the cloned data is still accessible

@kylebarron kylebarron merged commit d453028 into main Nov 28, 2023
@kylebarron kylebarron deleted the kyle/hard-clone branch November 28, 2023 04:20
@trevorgerhardt
Copy link

Could try testing with the web-worker plugin: https://github.com/vitest-dev/vitest/tree/main/packages/web-worker. Not 100% sure it would suit your needs.

@kylebarron
Copy link
Member Author

Cool! I didn't know about that.

It does say

Transferring Buffer will not change its byteLength.

which means I don't think I'd be able to test what I want to test here. Maybe a better way to test this is just to check that the buffer backing the new Data instance is a different object

@trevorgerhardt
Copy link

Ah, yes, I now see that note. A "real" test might need automating running a browser like with Playwright.

Also, as you probably realize, this code doesn't appear GeoArrow specific. Could be useful to propose into the upstream Arrow-JS project?

(Note: I am casually investigating these tools and do not know the dynamics of the different projects.)

@kylebarron
Copy link
Member Author

actually TIL node has a structuredClone method that correctly detaches arraybuffers:
image

Yeah, it's not geoarrow specific, but contributing upstream to arrow-js has a lot of overhead, so I might as well incubate functionality here, so I can move more quickly, and maybe later integrate with arrowjs

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants