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

Allow concurrent usage of zip library for list, zip and unzip operations #4

Merged
merged 1 commit into from Dec 4, 2016

Conversation

Projects
None yet
2 participants
@error454
Copy link
Contributor

commented Dec 3, 2016

This is a first pass at fully concurrent usage of zip operations. So far I've tested with 17 concurrent unzip operations, the speed improvement is substantial for my use case.

One thing I'd invite discussion on is the usage of InternalCallback->SetFlags(RF_MarkAsRootSet);. I was concerned that this will result in UZipFileFunctionInternalCallback never being GC'd even after the lambda is destroyed.

I'm guessing there may have been an original need to use this flag to prevent the pointer from getting GC'd, but were there certain circumstances around this? Really long running operations for instance? I've been testing without setting this flag but my general use-case is unzipping 1 file at a time so I may not be prone to hitting this. I was hoping with this refactor that the scoping is a little tighter and perhaps GC worries will go away.

@getnamo

This comment has been minimized.

Copy link
Owner

commented Dec 4, 2016

Looking through the code it appears some bits are missing, is the commit complete?

Regarding RF_MarkAsRoot, have you tried typing obj gc in the console or waited longer than a minute in any of your tests with it removed? I do agree that there is a cleaner way now that we're using handlers, perhaps creating the new objects owned by the delegate with IsValidLowLevel barriers to catch if the delegate or the world gets destroyed.

Edit: maybe it is complete, apparently I'm not familiar with my own code :). Will pull this and try it out first, but otherwise looks clean.

@getnamo getnamo merged commit fb2e688 into getnamo:master Dec 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.