Skip to content

Remove package variables for tracking handlers#5

Closed
edsrzf wants to merge 1 commit into
tecbot:masterfrom
edsrzf:remove-package-variables
Closed

Remove package variables for tracking handlers#5
edsrzf wants to merge 1 commit into
tecbot:masterfrom
edsrzf:remove-package-variables

Conversation

@edsrzf
Copy link
Copy Markdown
Contributor

@edsrzf edsrzf commented Apr 30, 2014

Previously various handlers were stored in maps with IDs, but because there was
no synchronization, it meant that some functions were not safe for concurrent
use.

Rather than add synchronization, I've removed the maps and used pointers
instead of IDs. This is concurrency-safe and cheaper since no
map lookups are required.

Previously various handlers were stored in maps with IDs, but because there was
no synchronization, it meant that some functions were not safe for concurrent
use.

Rather than add synchronization, I've removed the maps and used pointers
instead of IDs. This is concurrency-safe and cheaper since no
map lookups are required.
@tecbot
Copy link
Copy Markdown
Owner

tecbot commented Apr 30, 2014

Hi, the problem is that after the GC is executed the pointers doesn't work anymore (run the tests and you will see).

I tried many things in the past (also your solution) but this was the only way to solve it. You can read this post where I have described my problem: https://groups.google.com/forum/#!topic/golang-nuts/24BCnAWcqrw

@edsrzf
Copy link
Copy Markdown
Contributor Author

edsrzf commented Apr 30, 2014

Hmmm, I ran the tests locally and didn't have any problems. I guess I got lucky. The issue makes sense though.

Here's an alternative proposal, then (talking about comparators, but similar things would happen to merge operators and slice transforms):

  • Get rid of NewComparator and the Comparator type
  • Options.SetComparator accepts a ComparatorHandler and make the gorocks_comparator_create call
  • Options keeps a reference to the ComparatorHandler that was passed in
  • ComparatorHandler's name could optionally change to Comparator

This keeps the interfaces alive and safe from being collected as long as the Options struct lives, and since the DB struct keeps a pointer to Options, they'll live as long as the DB is alive.

@tecbot
Copy link
Copy Markdown
Owner

tecbot commented Apr 30, 2014

I tried your proposal but the funny thing is that hold a ref to the handler did not help. I tried after this to save the pointer and lo and behold, it worked!

You ok with my implementation?53c433f

@edsrzf
Copy link
Copy Markdown
Contributor Author

edsrzf commented Apr 30, 2014

Yeah, you'd have to save a pointer to the interface instead of just a copy of the interface, since you need to make sure the interface value itself also survives.

I do like the idea of getting rid of the Comparator struct type and having only an interface, but it's up to you what you want to do.

@tecbot
Copy link
Copy Markdown
Owner

tecbot commented Apr 30, 2014

Ok, for now I will merge only the pointer stuff.

Your idea is good, its reduce one extra step to create custom handlers and simplifies the API.
I will change it next time, or if you have free time you can do it self :)

tecbot added a commit that referenced this pull request Apr 30, 2014
Remove package variables (based on #5)
@tecbot
Copy link
Copy Markdown
Owner

tecbot commented Apr 30, 2014

Merged by #7

@tecbot tecbot closed this Apr 30, 2014
comstud pushed a commit to comstud/gorocksdb that referenced this pull request Jul 18, 2018
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.

2 participants