Skip to content
This repository has been archived by the owner on Jan 17, 2022. It is now read-only.

Encode enhancement #33

Merged
1 commit merged into from
Oct 7, 2016
Merged

Encode enhancement #33

1 commit merged into from
Oct 7, 2016

Conversation

lamphamsy
Copy link
Contributor

For encoding a single object, it does not need to copy the object to make sure
that it won't be deleted by garbage collector.
In the case of encoding an array of buffers, a memory copy is still necessary.

~AsyncEncodeWorker() {
delete _orig_data;
}
~AsyncEncodeWorker() {}
Copy link

Choose a reason for hiding this comment

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

Don't we need to reset the persistent flag in the destructor?

Copy link

@ghost ghost Sep 2, 2016

Choose a reason for hiding this comment

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

NanDisposePersistent() is apparently the way to go. That'll prevent leaks. Never trust old knowledge when it comes to v8 internals.

The persistent handle is dealt with in the destructor of AsyncWorker: https://github.com/nodejs/nan/blob/c212a7ca266e16efb2a8581b35f45ed72148b4f0/nan.h#L1484

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really understand your question. Is it about _persistent_flag? If yes, we don't need to do that

@ghost
Copy link

ghost commented Sep 2, 2016

One small nit, could we get rid of the flag itself (persistent_flag)? It has no real purpose and would mean an API breaking change.

@ghost
Copy link

ghost commented Sep 6, 2016

👍

@ghost ghost force-pushed the perf/not_copy_buffer branch from 22e04f1 to aa0f862 Compare October 7, 2016 14:50
For encoding a single object, it does not need to copy the object to make sure
that it won't be deleted by garbage collector.
In the case of encoding an array of buffers, a memory copy is still necessary.
@ghost
Copy link

ghost commented Oct 7, 2016

Merging, as this is a straight improvement, thanks again!

@ghost ghost merged commit 27a07ef into master Oct 7, 2016
@ghost ghost deleted the perf/not_copy_buffer branch October 7, 2016 15:05
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant