Skip to content

Conversation

@chrisseaton
Copy link
Contributor

@chrisseaton chrisseaton commented Jun 17, 2020

Allows you to go from

struct = CStructEntity.malloc([TYPE_INT, TYPE_LONG])
begin
  ...
ensure
  Fiddle.free struct
end

to

CStructEntity.malloc([TYPE_INT, TYPE_LONG], Fiddle::RUBY_FREE) do |struct|
  ..
end

and from

timeval = LIBC::Timeval.malloc()
begin
  timezone = LIBC::Timezone.malloc()
  begin
    LIBC.gettimeofday(timeval, timezone)
  ensure
    Fiddle.free timezone.to_ptr
  end
ensure
  Fiddle.free timeval.to_ptr
end

to

LIBC::Timeval.malloc(Fiddle::RUBY_FREE) do |timeval|
  LIBC::Timezone.malloc(Fiddle::RUBY_FREE) do |timezone|
    LIBC.gettimeofday(timeval, timezone)
  end
end

Both are safer, have better temporal and spatial locality for the memory manager, and reduce the working set size.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

Thanks.
I'll review this deeply later.

addr = Fiddle.malloc(CStructEntity.size(types))
CStructEntity.new(addr, types, func)
addr = Fiddle.malloc(self.size(types))
struct = new(addr, types, func)
Copy link
Member

Choose a reason for hiding this comment

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

If an exception is raised in here, addr isn't freed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah maybe I should use the pointer constructor rather than using malloc directly as it used to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

I understand that we need reconstruct Fiddle::Pointer and lib/fiddle/struct.rb.
It may be better that we merge this as-is and work on it as a follow-up task.

end
end

def test_new_double_free
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@chrisseaton
Copy link
Contributor Author

This is the end of this series of PRs tweaking the API.

I do have some ideas about how to improve the implementation. I've got some goals to be able to share code between Fiddle and FFI, in order to make it easier for alternative implementations of Ruby which don't want to have to implement both interfaces. And I've got some goals to allow pointer operations to optimise much better in optimising implementations of Ruby.

end
}
size = klass.entity_class.size(types)
# we use eval here make size into a literal, for performance
Copy link
Member

Choose a reason for hiding this comment

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

We can use define_singleton_method for size here because this method will not be called too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

addr = Fiddle.malloc(CUnionEntity.size(types))
CUnionEntity.new(addr, types, func)
super
end
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add it - but removed as you've asked.

chrisseaton and others added 6 commits June 21, 2020 22:29
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
@chrisseaton chrisseaton requested a review from kou June 21, 2020 21:37
@kou kou merged commit 9836100 into ruby:master Jun 21, 2020
@kou
Copy link
Member

kou commented Jun 21, 2020

Thanks!

@chrisseaton chrisseaton deleted the struct-union-pointer branch June 21, 2020 22:00
kojix2 added a commit to kojix2/LibUI that referenced this pull request Oct 15, 2021
kojix2 added a commit to kojix2/LibUI that referenced this pull request Oct 15, 2021
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