-
Notifications
You must be signed in to change notification settings - Fork 201
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 a GC compaction issue with busy_handler
#466
Fix a GC compaction issue with busy_handler
#466
Conversation
NULL, | ||
deallocate, | ||
database_memsize, | ||
.wrap_struct_name = "SQLite3::Backup", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question from ignorance:
Peter's article describes the wrap_struct_name
this way:
This is a C string name we give for our TypedData object. This name is used for debugging and statistics purposes. Although there are no requirements for this name (it just has to be a valid C string that’s null-terminated), it is a good idea to give it an informative and unique name to make it easy for yourself and other developers to identify the object.
For my own edification, why do we use the same string for 3 different TypedData objects:
https://github.com/search?q=repo%3Asparklemotion%2Fsqlite3-ruby%20rb_data_type_t&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea. I personally tend to set it as the name of the class it backs, but as Peter says, it can be anything.
Ultimately this doesn't have a big impact, the only way to see it is via ObjectSpace.dump
and ObjectSpace.dump_all
so is only useful when debugging memory related things.
Would probably be worth reworking them all, but I don't own that gem, so not my call.
Previous discussion in sparklemotion#458 Storing `VALUE self` as context for `rb_sqlite3_busy_handler` is unsafe because as of Ruby 2.7, the GC compactor may move objects around which can lead to this reference pointing to either another random object or to garbage. Instead we can store the callback reference inside the malloced struct (`sqlite3Ruby`) which can't possibly move, and then inside the handler, get the callback reference from that struct. This however requires to define a mark function for the database object, and while I was at it, I implemented compaction support for it so we don't pin that proc.
b024d9b
to
e46e1d2
Compare
Previous commit was failing on Truffle with:
Which is weird because looking at the Truffle source, that function very much seem to be defined, so I don't quite get it (FYI @eregon). But overall, making that proc movable isn't that important, we're talking about one object per connection, so likely very few in each process, so not really worth checking for the function existence etc, I simply removed the compaction support and let the callback be pinned. |
Now valgrind is failing, but the backtrace is inside |
Re |
Ah! My bad, I saw it it the source from a fairly old commit and I assumed it was there. Thanks @eregon (and sorry for not looking more attentively). |
Ugh yep. This PR makes sense. I don't see how this is related to the valgrind test either, so I'm going to merge. |
Previous discussion in #458
Storing
VALUE self
as context forrb_sqlite3_busy_handler
is unsafe because as of Ruby 2.7, the GC compactor may move objects around which can lead to this reference pointing to either another random object or to garbage.Instead we can store the callback reference inside the malloced struct (
sqlite3Ruby
) which can't possibly move, and then inside the handler, get the callback reference from that struct.This however requires to define a mark function for the database object
, and while I was at it, I implemented compaction support for it so we don't pin that proc.cc @flavorjones @tenderlove