Skip to content

Implement embedded TypedData objects #7440

Merged
peterzhu2118 merged 2 commits intoruby:masterfrom
Shopify:pz-embedded-typed-data
Nov 7, 2023
Merged

Implement embedded TypedData objects #7440
peterzhu2118 merged 2 commits intoruby:masterfrom
Shopify:pz-embedded-typed-data

Conversation

@peterzhu2118
Copy link
Member

@peterzhu2118 peterzhu2118 commented Mar 3, 2023

This PR adds a new flag RUBY_TYPED_EMBEDDABLE that allows the data of a TypedData object to be embedded after the object itself. This will improve cache locality and allow us to save the 8 byte data pointer.

This PR also implements this feature on Time objects, which drops the total size of a Time object from 86 bytes to 80 bytes.

This PR is based on top of #7438.

Co-Authored-By: @byroot

@peterzhu2118 peterzhu2118 marked this pull request as draft March 3, 2023 21:11
@peterzhu2118 peterzhu2118 force-pushed the pz-embedded-typed-data branch 4 times, most recently from da31399 to 6e65bee Compare March 6, 2023 20:24
@casperisfine
Copy link
Contributor

casperisfine commented Mar 9, 2023

Hum, it just hit me: what is the impact on compaction?

T_DATA are movable right? given that they only contain a pointer.

But if we embed the struct, and the extension pass pointers to members of the struct to various C APIs, moving the struct might break, right?

@peterzhu2118
Copy link
Member Author

peterzhu2118 commented Mar 9, 2023

Since this is opt-in and as long as we don't store the pointer anywhere, then it should be safe for compaction (assuming that the object is also on the stack using RB_GC_GUARD). I should revisit the code in time.c to ensure that all the RB_GC_GUARD are there.

@peterzhu2118 peterzhu2118 force-pushed the pz-embedded-typed-data branch from 6e65bee to 54915d0 Compare March 9, 2023 15:50
@peterzhu2118
Copy link
Member Author

peterzhu2118 commented Mar 9, 2023

I added a bunch of RB_GC_GUARD. I think even without embedding TypedData, some functions need RB_GC_GUARD for correctness.

@peterzhu2118 peterzhu2118 force-pushed the pz-embedded-typed-data branch from 54915d0 to c3f5db7 Compare March 9, 2023 15:52
@byroot byroot mentioned this pull request Jun 19, 2023
Merged
@peterzhu2118 @byroot
Implement embedded TypedData objects
ef01168 
This commit adds a new flag RUBY_TYPED_EMBEDDABLE that allows the data
of a TypedData object to be embedded after the object itself. This will
improve cache locality and allow us to save the 8 byte data pointer.

Co-Authored-By: Jean Boussier <byroot@ruby-lang.org>
@casperisfine casperisfine force-pushed the pz-embedded-typed-data branch from c3f5db7 to 88034f0 Compare November 7, 2023 14:47
@byroot byroot marked this pull request as ready for review November 7, 2023 14:49
@casperisfine
Copy link
Contributor

casperisfine commented Nov 7, 2023 

/opt/rubies/3.2.2/bin/ruby --disable=gems -rrubygems -I./benchmark/lib ./benchmark/benchmark-driver/exe/benchmark-driver \
	            --executables="compare-ruby::../miniruby-master -I.ext/common --disable-gem" \
	            --executables="built-ruby::./miniruby --disable-gem" \
	            --output=markdown --output-compare -v $(find ./benchmark -maxdepth 1 -name 'time_now' -o -name '*time_now*.yml' -o -name '*time_now*.rb' | sort) 
compare-ruby: ruby 3.3.0dev (2023-11-07T14:19:51Z master 1cfc853be6) [arm64-darwin22]
last_commit=Suppress nonnull warning from gcc 13
built-ruby: ruby 3.3.0dev (2023-11-07T14:43:41Z pz-embedded-typed-.. 88034f0318) [arm64-darwin22]
warming up..
# Iteration per second (i/s)

|                        |compare-ruby|built-ruby|
|:-----------------------|-----------:|---------:|
|Time.now                |     12.150M|   22.025M|
|                        |           -|     1.81x|
|Time.now(in: "+09:00")  |      3.545M|    4.794M|
|                        |           -|     1.35x|

@peterzhu2118 peterzhu2118 force-pushed the pz-embedded-typed-data branch from 88034f0 to e335dfa Compare November 7, 2023 18:24
@peterzhu2118 @byroot
Use embedded TypedData for Time objects
1331e72 
This drops the total size of a Time object from 86 bytes to 80 bytes.

Running the benchmark benchmark/time_now.yml, this commit improves
performance of Time.now by about 30%:

```
  Time.now
Branch:  13159405.4 i/s
Master:  10036908.7 i/s - 1.31x  slower

  Time.now(in: "+09:00")
Branch:   2712172.6 i/s
Master:   2138637.9 i/s - 1.27x  slower
```

It also decreases memory usage by about 20%:

```
ary = 10_000_000.times.map { Time.now }

puts `ps -o rss= -p #{$$}`
```

Branch: 961792
Master: 1196544

Co-Authored-By: Jean Boussier <byroot@ruby-lang.org>
@peterzhu2118 peterzhu2118 force-pushed the pz-embedded-typed-data branch from e335dfa to 1331e72 Compare November 7, 2023 18:31
@peterzhu2118 peterzhu2118 merged commit aa6642d into ruby:master Nov 7, 2023
@peterzhu2118 peterzhu2118 deleted the pz-embedded-typed-data branch November 7, 2023 20:48
@casperisfine
Copy link
Contributor

casperisfine commented Nov 8, 2023

Seems like we introduced an issue for some gems:

/usr/local/ruby/include/ruby-3.3.0+0/ruby/internal/core.h:34,
                 from /usr/local/ruby/include/ruby-3.3.0+0/ruby/ruby.h:29,
                 from /usr/local/ruby/include/ruby-3.3.0+0/ruby.h:38,
                 from heap_profiler.cpp:1:
heap_profiler.cpp: In function ‘VALUE parser_allocate(VALUE)’:
/usr/local/ruby/include/ruby-3.3.0+0/ruby/internal/core/rtypeddata.h:467:33:
error: invalid conversion from ‘void*’ to ‘parser_t*’ [-fpermissive]
  467 |     (sval) = RTYPEDDATA_GET_DATA(result); \
      |              ~~~~~~~~~~~~~~~~~~~^~~~~~~~
      |                                 |
      |                                 void*
/usr/local/ruby/include/ruby-3.3.0+0/ruby/internal/core/rtypeddata.h:487:9:
note: in expansion of macro ‘TypedData_Make_Struct0’
  487 |         TypedData_Make_Struct0( \
      |         ^~~~~~~~~~~~~~~~~~~~~~
heap_profiler.cpp:33:17: note: in expansion of macro ‘TypedData_Make_Struct’
33 |     VALUE obj = TypedData_Make_Struct(klass, parser_t,
&parser_data_type, data);
      |                 ^~~~~~~~~~~~~~~~~~~~~
make: *** [Makefile:240: heap_profiler.o] Error 1

I'll try to figure this out.

@casperisfine casperisfine mentioned this pull request Nov 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@peterzhu2118 @casperisfine