Add a Fiddle::Pointer#call_free method #36

chrisseaton · 2020-05-31T21:19:01Z

free! runs the user's free function. It doesn't do anything if the pointer has already been freed. The GC won't call the user's free function if the user already manually freed. Another method freed? tells the user if the pointer has already been freed.

All together, these changes allow you to rely on the GC to definitely free your memory, meaning no leaks, but then also allows you to manually free as early as possible if you can, meaning deterministic deallocation, improving cache locality and and reducing peaks in memory use.

chrisseaton · 2020-05-31T21:27:18Z

ext/fiddle/pointer.c

@@ -24,6 +24,7 @@ struct ptr_data {
    void *ptr;
    long size;
    freefunc_t free;
+    int freed;


Is int and 1 and 0 the right way to do booleans in C extensions? I found some other things but wasn't sure if they were internal for MRI?

We can use bool and true and false.

This failed in all CI configurations when I tried it.

We need #include <stdbool.h>.

chrisseaton · 2020-05-31T21:28:31Z

Some of these Windows CI failures don't make sense to me - some say they can't find libffi and some look like they're testing the system Fiddle, not the Fiddle in this repository.

chrisseaton · 2020-05-31T21:30:06Z

test/fiddle/test_pointer.rb

+      ptr.free!
+      assert ptr.freed?
+      ptr.free!                 # you can safely run it again
+      assert ptr.freed?


What do we think free! and freed? should do when there's no free function attached?

How about doing nothing?

refute ptr.freed? ptr.free! refute ptr.freed? ptr.free! refute ptr.freed?

Yes that's what we do at the moment. As long as everyone else also thinks that's fine I'll do that.

kou

We can ignore the "windows mswin" failure for now.
See also: #35

kou · 2020-05-31T21:57:27Z

ext/fiddle/pointer.c

@@ -24,6 +24,7 @@ struct ptr_data {
    void *ptr;
    long size;
    freefunc_t free;
+    int freed;


We can use bool and true and false.

kou · 2020-05-31T22:02:26Z

test/fiddle/test_pointer.rb

+      begin
+        assert_equal 10, ptr.size
+        assert_equal free.to_i, ptr.free.to_i
+      ensure


Why do we need this?
The ptr is freed by GC when on of the above two assertions is failed, isn't it?

In my mind, it's good practice to free a native, external resource as soon as you can. That's why we use blocks for example when opening a file, rather than letting the GC close it. In the future I hope to create a PR for a similar block scope for pointers as well.

OK.
Could you revert these changes in this pull request? We can rewrite our test codes with the block scope free feature in the pull request.

Remove the manual free, leave it relying on GC for now, and then switch to the block malloc that's coming in the next PR? Ok.

kou · 2020-05-31T22:03:58Z

test/fiddle/test_pointer.rb

@@ -20,16 +20,28 @@ def test_malloc_free_func_int
      assert_equal free.to_i, Fiddle::RUBY_FREE.to_i

      ptr  = Pointer.malloc(10, free.to_i)
-      assert_equal 10, ptr.size
-      assert_equal free.to_i, ptr.free.to_i
+      refute ptr.freed?


Do we need test freed? here?
It seems that you add separated tests for it.

kou · 2020-05-31T22:05:46Z

test/fiddle/test_pointer.rb

+      ptr.free!
+      assert ptr.freed?
+      ptr.free!                 # you can safely run it again
+      assert ptr.freed?


How about doing nothing?

refute ptr.freed? ptr.free! refute ptr.freed? ptr.free! refute ptr.freed?

kou · 2020-05-31T22:06:31Z

test/fiddle/test_pointer.rb

+      ptr = Pointer.malloc(4, Fiddle::RUBY_FREE)
+      refute ptr.freed?
+      ptr.free!
+      assert ptr.freed?
+      ptr.free!                 # you can safely run it again
+      assert ptr.freed?
+      GC.start                  # you can safely run the GC routine
+      assert ptr.freed?


Could you split this case to another test?

kou · 2020-05-31T22:11:51Z

ext/fiddle/pointer.c

@@ -370,6 +385,34 @@ rb_fiddle_ptr_free_get(VALUE self)
    return rb_fiddle_new_function(address, arg_types, ret_type);
 }

+/*
+ * call-seq: free! => nil


How about renaming to call_free or something?
free (getter) and free! will confuse us.

Yes... I was going for terseness, but I'm open to debating this.

It's also a destructive operation, which fits the ! pattern.

free! is destructive but free! is not the destructive version of free.
The relation of them is different from the relation of String#gsub and String#gsub!.

I feel like free! is best due to being short and looking destructive. I'm open to changing it if you're very certain or other people agree.

If we have block style free feature (Fiddle::Pointer.malloc(size, Fiddle::RUBY_FREE) {|pointer| ...}?), we'll not call this method manually.
So I don't think we should use shorter name for this method with potential confusion. (Some users may use pointer.free wrongly to free a pointer but the pointer isn't freed. It causes a memory leak.)

Ok I'll use call_free.

kou · 2020-05-31T22:15:16Z

We can also ignore all Windows jobs...

MSP-Greg · 2020-06-01T15:40:11Z

@chrisseaton @kou

I apologize for not looking at the Windows logs in Actions.

All Windows MinGW Ruby versions, 2.4 and later, have a vendored file, rubygems/defaults/operating_system.rb, that then requires files that eventually require fiddle. It's used for a win32 call that adds folders for dll resolution without adding the folder to PATH.

See #37, which disables the loading of operating_system.rb via --disable=gems...

EDIT: I just created a branch with this PR, then added the change in PR #37, and all tests passed (except mswin).

See Actions run:
https://github.com/MSP-Greg/fiddle/runs/727852744
branch commits:
https://github.com/MSP-Greg/fiddle/commits/pr36-win

ext/fiddle/pointer.c

kou · 2020-06-03T00:20:30Z

Thanks!

Add a Fiddle::Pointer#free! method

7100f65

chrisseaton force-pushed the free! branch from e658faf to 7100f65 Compare May 31, 2020 21:23

chrisseaton commented May 31, 2020

View reviewed changes

kou requested changes May 31, 2020

View reviewed changes

chrisseaton added 4 commits June 2, 2020 14:25

Use stdbool

270de77

Fiddle::Pointer#free! does nothing if no free function installed

cb45997

Tidy up Fiddle::Pointer specs

d9de7bc

Improve Fiddle::Pointer docs

03f6f80

chrisseaton requested a review from kou June 2, 2020 13:56

chrisseaton added 2 commits June 2, 2020 23:42

Rename Fiddle::Pointer#free! to #call_free

a5c6497

Don't manually free pointers in tests for now

25307ec

chrisseaton force-pushed the free! branch from 972370d to 25307ec Compare June 2, 2020 22:45

kou changed the title ~~Add a Fiddle::Pointer#free! method~~ Add a Fiddle::Pointer#call_free method Jun 3, 2020

kou reviewed Jun 3, 2020

View reviewed changes

ext/fiddle/pointer.c Outdated Show resolved Hide resolved

free! -> call_free

7c852e8

kou reviewed Jun 3, 2020

View reviewed changes

ext/fiddle/pointer.c Outdated Show resolved Hide resolved

free! -> call_free

fd13082

kou reviewed Jun 3, 2020

View reviewed changes

ext/fiddle/pointer.c Outdated Show resolved Hide resolved

free! -> call_free

6d6b140

kou merged commit db097cb into ruby:master Jun 3, 2020

chrisseaton deleted the free! branch June 10, 2020 20:28

chrisseaton mentioned this pull request Aug 31, 2020

Release and merge into MRI #45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Fiddle::Pointer#call_free method #36

Add a Fiddle::Pointer#call_free method #36

chrisseaton commented May 31, 2020

chrisseaton May 31, 2020

kou May 31, 2020

chrisseaton May 31, 2020

kou May 31, 2020

chrisseaton Jun 2, 2020

chrisseaton commented May 31, 2020

chrisseaton May 31, 2020

kou May 31, 2020

chrisseaton May 31, 2020

chrisseaton Jun 2, 2020

kou left a comment

kou May 31, 2020

kou May 31, 2020

chrisseaton May 31, 2020

kou Jun 2, 2020

chrisseaton Jun 2, 2020

chrisseaton Jun 2, 2020

kou Jun 3, 2020

kou May 31, 2020

chrisseaton Jun 2, 2020

kou May 31, 2020

kou May 31, 2020

chrisseaton Jun 2, 2020

kou May 31, 2020

chrisseaton May 31, 2020

chrisseaton May 31, 2020

kou May 31, 2020

chrisseaton Jun 2, 2020

kou Jun 2, 2020

chrisseaton Jun 2, 2020

chrisseaton Jun 2, 2020

kou commented May 31, 2020

MSP-Greg commented Jun 1, 2020 •

edited

Loading

kou commented Jun 3, 2020

Add a Fiddle::Pointer#call_free method #36

Add a Fiddle::Pointer#call_free method #36

Conversation

chrisseaton commented May 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisseaton commented May 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented May 31, 2020

MSP-Greg commented Jun 1, 2020 • edited Loading

kou commented Jun 3, 2020

MSP-Greg commented Jun 1, 2020 •

edited

Loading