Skip to content

Implement Factory#pool - #266

Merged
tagomoris merged 1 commit into
msgpack:masterfrom
Shopify:factory-pool
Feb 23, 2022
Merged

Implement Factory#pool#266
tagomoris merged 1 commit into
msgpack:masterfrom
Shopify:factory-pool

Conversation

@casperisfine

@casperisfine casperisfine commented Feb 14, 2022

Copy link
Copy Markdown

Fix: #255

When handling small payload creating the Packer and Unpacker
instance can easily account for the majority of the serialization
and deserialization time.

For this reason it is recommended to reuse these objects, and
Factory#pool is a helper for that.

@casperisfine casperisfine mentioned this pull request Feb 14, 2022
Closed
@casperisfine
casperisfine force-pushed the factory-pool branch 3 times, most recently from 138a184 to 108d343 Compare February 15, 2022 12:14
@casperisfine

casperisfine commented Feb 15, 2022

Copy link
Copy Markdown
Author

As always I forgot to check JRuby was passing. It should be fixed now.

casperisfine
casperisfine commented Feb 16, 2022
View reviewed changes
Comment thread ext/msgpack/factory_class.c
FACTORY(self, fc);

if (RTEST(fc->pkrg.hash)) {
rb_hash_freeze(fc->pkrg.hash);

@casperisfine casperisfine Feb 16, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I squeezed an extra optimization here. When we freeze the factory, the types registry hash gets frozen as well. This way all packers can share it safely.

tagomoris
tagomoris reviewed Feb 22, 2022
View reviewed changes

@tagomoris tagomoris left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The implementation looks good to me.
The name PooledFactory looks misleading. That name looks to mean to pool Factory instances, not Packers nor Unpackers.
I don't have any good idea though...

Comment thread ext/msgpack/factory_class.c
Comment thread lib/msgpack/pooled_factory.rb Outdated
Comment thread lib/msgpack/pooled_factory.rb Outdated
Comment thread lib/msgpack/factory.rb Outdated
@casperisfine
casperisfine force-pushed the factory-pool branch 4 times, most recently from c27668d to 5f66b4a Compare February 22, 2022 13:47
@casperisfine

casperisfine commented Feb 22, 2022

Copy link
Copy Markdown
Author

All comments addressed I think.

As for PooledFactory, it's meant to be used through Factory#pool, so I believe it's not too big a deal?

@tagomoris

tagomoris commented Feb 23, 2022

Copy link
Copy Markdown
Member

How about MessagePack::Factory::Pool? It means Factory#pool in a straightforward way (in my opinion).

@byroot
Implement Factory#pool
5f9e9f4 
Fix: msgpack#255

When handling small payload creating the Packer and Unpacker
instance can easily account for the majority of the serialization
and deserialization time.

For this reason it is recommanded to reuse these objects, and
`Factory#pool` is a helper for that.
@casperisfine

casperisfine commented Feb 23, 2022

Copy link
Copy Markdown
Author

How about MessagePack::Factory::Pool?

Done. Also since the class isn't that big I moved it inside factory.rb, but I can split it if you prefer.

@tagomoris

tagomoris commented Feb 23, 2022

Copy link
Copy Markdown
Member

LGTM. Thank you!

@tagomoris
tagomoris merged commit 04ccbd8 into msgpack:master Feb 23, 2022
This was referenced Mar 8, 2022
Closed
Merged
@byroot byroot mentioned this pull request Oct 28, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tagomoris tagomoris tagomoris left review comments

Assignees

No one assigned

Labels

new feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Idea: Specialized bufferless Unpacker for strings

3 participants

@casperisfine @tagomoris @byroot