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

Change all T_DATA objects to typed data #349

Merged
merged 9 commits into from
Aug 30, 2020
Merged

Conversation

larskanis
Copy link
Collaborator

@larskanis larskanis commented Aug 6, 2020

The classic way to allocate T_DATA objects is deprecated since Ruby-2.0. The new way with rb_data_type_t structure has same advantages:

  1. Type checks are more specific
  2. C-ext can emit memory size information
  3. C objects can be made GC.compact friendly

Currently TruffleRuby fails in CI, but the issue is fixed in oracle/truffleruby@f460ce7

This makes ruby-pg GC.compact friendly by reverting 7c1756f but using the compactation callback. This hopefully improves performance as described in #343.

Classic T_DATA objects are deprecated since ruby-2.0.
On the other hand TypedData objects can be checked more easy and allows some new features.
All T_DATA objects of ruby-pg are based on TypedData_Struct now.
So we're no longer using deprecated non-typed objects.
This way the VALUE references may be relocated.
Since self is always referenced indirectly, marking simple coders can be omitted entirely.

This partly reverts 7c1756f which was introduced as a simple fix for GC.compact compatbility.
This way the VALUE references may be relocated.

This partly reverts 7c1756f which was introduced as a simple fix for GC.compact compatbility.
Comment on lines 6 to 7
if GC.respond_to?(:compact)
describe "GC.compact" do
Copy link
Contributor

Choose a reason for hiding this comment

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

You may be able to indent less using RSpec's built-in filtering.

Suggested change
if GC.respond_to?(:compact)
describe "GC.compact" do
describe "GC.compact", if: GC.respond_to?(:compact) do

Copy link
Contributor

Choose a reason for hiding this comment

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

The filtering is a bit weird though as it still executes the block.
Probably fine here since all the logic is in before/it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

describe blocks are still executed in case of if: false. Only the execution of it blocks is omitted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved all code into before, it and after blocks and made use of the :if filtering. Thanks for the hint!

@@ -0,0 +1,85 @@
# -*- rspec -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

This file seems to use tabs, but all other specs use the conventional 2-spaces indent, could you re-indent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, all ruby-pg source files use tabs indent (for some reason).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I mixed with FFI, sorry.

In the hope that the issue with TypedData inheritance is fixed there.
It is supported since ruby-2.1 and pg supports ruby-2.2+
@conninfo is not always defined in a describe context
So define the constants in a before block and ensure the connection is closed.

Moreover use rspec's :if filter to avoid indention
"if: false" skips the execution of before, after and it blocks but the describe block is always executed.
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

3 participants