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

fix: perf, revert array, shift #10

Merged
merged 3 commits into from
Mar 9, 2020
Merged

fix: perf, revert array, shift #10

merged 3 commits into from
Mar 9, 2020

Conversation

hugomrdias
Copy link
Owner

@hugomrdias hugomrdias commented Mar 8, 2020

/cc @MaxGraey
do you have any idea why the Array version doesn't work?
also i can't find any docs in the @inline decorators were they removed ?

src/rabin.js Outdated
Comment on lines 50 to 56
const cleanArr = []
for (let i = 0; i < processed.length; i++) {
if(processed[i] === 0) break
cleanArr[i] = processed[i];
}

return cleanArr
Copy link
Contributor

@MaxGraey MaxGraey Mar 8, 2020

Choose a reason for hiding this comment

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

Suggested change
const cleanArr = []
for (let i = 0; i < processed.length; i++) {
if(processed[i] === 0) break
cleanArr[i] = processed[i];
}
return cleanArr
const end = processed.indexOf(0);
return end >= 0 ? processed.subarray(0, end) : processed;

src/rabin.js Outdated
Comment on lines 47 to 48
__release(lengthsPtr)
__release(pointer)
Copy link
Contributor

@MaxGraey MaxGraey Mar 8, 2020

Choose a reason for hiding this comment

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

Much better swap this. So alloc and release will be in LIFO order.

Suggested change
__release(lengthsPtr)
__release(pointer)
__free(pointer)
__free(lengthsPtr)

Because currently used stub runtime, not full or half

Copy link
Owner Author

Choose a reason for hiding this comment

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

I changed to full, why __free what's the difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

All this described in docs: https://docs.assemblyscript.org/details/runtime#runtime-variants

in short full / half use RC/GC runtime, stub/none just manual memory managment and retain / release just stubs in this case

Copy link
Owner Author

Choose a reason for hiding this comment

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

Read that already I just can't find the __free function, I would prefer to use stub and manage memory manually.

Copy link
Owner Author

Choose a reason for hiding this comment

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

With full you still need to tell wasm to __retain and after __release so it's kinda manual.
Any docs on how to go full manual with stub/none runtime?

Copy link
Contributor

Choose a reason for hiding this comment

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

With full you still need to tell wasm to __retain and after __release

It's only for js / host side because we can't handle in this case memory automatically. But internally (in AS) you don't need call release / retain because compiler do this by himself (same as in Swift which used ARC)

@MaxGraey
Copy link
Contributor

MaxGraey commented Mar 8, 2020

do you have any idea why the Array version doesn't work?

Array has slightly different layout and require __getArray instead __getUint32Array. But better use typed arrays when pass between host and wasm. It usually has faster interop.

@hugomrdias hugomrdias merged commit b9405d8 into master Mar 9, 2020
@hugomrdias hugomrdias deleted the fix/revert-array branch March 9, 2020 17:08
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.

None yet

2 participants