Add ability to use symbols as field names instead of strings by larskanis · Pull Request #306 · ged/ruby-pg

larskanis · 2019-11-10T20:41:31Z

This adds:

PG::Result#field_name_type=
PG::Result#field_name_type
PG::Result#field_names_as
PG::Connection#field_name_type=
PG::Connection#field_name_type

Comparison and lookup is faster on symbols than on strings, since the string hash of symbols is generated at Symbol creation. So retrieval of Symbol based field names is somewhat slower, but this is easily outperformed in frameworks that do a lot of lookups.

There are currently two ways to create hashes in C: dynamic and static symbols. Dynamic symbols are GC'ed and static symbols aren't. I added both ways to this PR. Non-GC'able symbols can have unexpected memory bloat, when field names are dynamically generated. Since both ways showed comparable timings, I think we could remove static symbols again.

Mysql2 implements symbol keys since ages, but only as static symbols. Maybe the possible memory bloat is a reason why symbolize_keys isn't used in ActiveRecord.

This raises ruby version requirement to 2.2, since it depends on GC'able dynamic symbols.

Fixes #278

cbandy

Tests look good.

cbandy · 2019-11-11T02:21:12Z

 	return this->pgresult;
 }

+static VALUE pg_cstr_to_sym(char *cstr, unsigned int flags, int enc_idx)


The signature of this function could be more clear that it is specific to field names.

Shrug! I don't care much about static function names.

cbandy · 2019-11-11T02:27:08Z

+			this->fnames[i] = pg_cstr_to_sym(cfname, this->flags, this->enc_idx);
 			this->nfields = i + 1;
-
-			RB_GC_GUARD(fname);


I suppose the call to (or return from?) pg_cstr_to_sym makes this redundant?

A static function like pg_cstr_to_sym() could be inlined by the compiler, so it's not safe to protect the VALUE from being GC'ed. However a call to a non-static function is sufficient to protect it from being GC'ed, so the call to rb_obj_freeze() is (and was) already enough.

cbandy · 2019-11-11T02:33:18Z

- *    res.fname( index ) -> String
+ *    res.fname( index ) -> String or Symbol
 *
 * Returns the name of the column corresponding to _index_.


It seems worth mentioning field_name_type in these cases where "string or symbol" may be returned.

Yes, I'll change this.

cbandy · 2019-11-11T02:35:05Z

+ * It can be set to one of:
+ * * +:string+ to use String based field names
+ * * +:symbol+ to use Symbol based field names
+ * * +:static_symbol+ to use pinned Symbol (can not be garbage collected) - Don't use this, it will probably removed in future.


Why deprecate :static_symbol the moment it is introduced? Is there a use case for having it?

:static_symbol is the classic way to create symbols from C and it is also used by mysql2 gem. So I wanted to be able compare and benchmark it a bit more. So far I think it is mostly obsolete.

cbandy · 2019-11-11T02:40:53Z

 	VALUE result = pg_new_result(this->pgresult, this->connection);
+	t_pg_result *newthis = pgresult_get_this(result);
 	UNUSED(nfields);
+	newthis->flags = this->flags;


I don't grok pg_new_result, so this is a naive question: should this copy happen inside pg_new_result?

Hmm, good question! If I think about it a bit, I notice that the initial struct values are copied from this->connection in pg_new_result, but they should be copied from this instead. So currently typemap= is broken on stream_each_tuple, unless it's already set on the connection. I'll think a bit more about it, how this can be solved best.

I fixed this issue in commit 6a450fa on the master branch by introducing a result copy function instead of reusing pg_new_result. Thank you for highlighting this!

larskanis · 2019-11-12T20:31:03Z

Thank you @cbandy for your valuable feedback! I'll come back with an improved version.

larskanis · 2019-11-17T18:13:29Z

@ged I would appreciate an opinion from you. Is the API OK for you?

ged · 2019-11-18T17:47:40Z

@larskanis : Yes, this looks good to me. Just to clarify: you're including static_symbol so you can benchmark against mysql2, but it will likely be removed in favor of just symbol?

larskanis · 2019-11-18T20:15:05Z

@ged : No, not to compare against mysql2, just to compare the two ways of symbol allocation. I noticed that String allocation is faster, but dynamic vs. static Symbol allocation seems to have mostly the same speed. I still kept both versions, so far, just to have them easily available. And as it turned out, the static symbols have to be used on Truffleruby (and they are dynamic there). I'll remove :static_symbol type, if you don't like it. Otherwise it can be removed similar to conn.guess_result_memsize= when we know that no one needs the alternative way.

What do you think about conn.field_name_type= to set a default field_name_type for results?

ged · 2019-11-18T20:54:56Z

Ah, okay. The method name makes sense to me, as does keeping the :static_symbol setting. 👍

larskanis · 2019-11-19T06:15:11Z

@ged The second question was more about adding field_name_type= to PG::Connection to set the default type per connection?

ged · 2019-11-19T14:59:50Z

Oh, then yes I think that'd be useful. I guess I took its utility for granted. ;)

This adds: * PG::Result#field_name_type= * PG::Result#field_name_type * PG::Result#field_names_as Symbol comparison and lookup is faster, since the string hash is generated at Symbol creation. So retrieval of Symbol based field names is somewhat slower, but this is easily outperformed in frameworks that do a lot of lookups. This raises ruby version requirement to 2.2, since it depends on GC'able symbols. Fixes ged#278

…apped values

Truffleruby doesn't support rb_check_symbol_cstr() so far, but all symbols in Truffleruby are GC'd anyway. So we can use classic ID2SYM to allocate the Symbol objects. Discussed here: truffleruby/truffleruby#1814

It allows to set the field name type on a connection scope. Added methods are: * PG::Connection#field_name_type= * PG::Connection#field_name_type

cbandy · 2019-11-20T13:19:37Z

+{
+	VALUE fname;
+	if( flags & PG_RESULT_FIELD_NAMES_SYMBOL ){
+#ifndef TRUFFLERUBY


The code here looks correct for :symbol and :string on truffle ruby, but it looks like :static_symbol will produce strings.

It was using strings before.

ged · 2019-11-22T00:20:52Z

Excellent work as usual sir! 👍

larskanis force-pushed the fields-as-symbol branch 2 times, most recently from d63dbd0 to 48f1163 Compare November 10, 2019 20:56

cbandy reviewed Nov 11, 2019

View reviewed changes

larskanis force-pushed the fields-as-symbol branch from 1191f40 to cb1047d Compare November 16, 2019 22:08

larskanis mentioned this pull request Nov 18, 2019

rb_check_symbol_cstr is not yet implemented truffleruby/truffleruby#1814

Closed

larskanis added 4 commits November 19, 2019 22:27

Update gemspec per rake gemspec

df0e74d

Add a link to methods that change behaviour depending on field_name_type

7adc26b

Make sure PG::Result#stream_each and stream_each_tuple delivers typem…

8fd166a

…apped values

larskanis force-pushed the fields-as-symbol branch 2 times, most recently from 0ae39a9 to fd66f35 Compare November 19, 2019 21:35

larskanis added 2 commits November 20, 2019 11:56

Fix compat with Truffleruby

57e4b6e

Truffleruby doesn't support rb_check_symbol_cstr() so far, but all symbols in Truffleruby are GC'd anyway. So we can use classic ID2SYM to allocate the Symbol objects. Discussed here: truffleruby/truffleruby#1814

Add field_name_type setter and getter to PG::Connection

fb49bba

It allows to set the field name type on a connection scope. Added methods are: * PG::Connection#field_name_type= * PG::Connection#field_name_type

larskanis force-pushed the fields-as-symbol branch from 330b4de to fb49bba Compare November 20, 2019 10:57

cbandy reviewed Nov 20, 2019

View reviewed changes

Fix :static_symbol on Truffleruby

dffb658

It was using strings before.

larskanis merged commit dffb658 into ged:master Nov 21, 2019

larskanis deleted the fields-as-symbol branch November 21, 2019 20:23

Conversation

larskanis commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbandy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larskanis commented Nov 12, 2019

Uh oh!

larskanis commented Nov 17, 2019

Uh oh!

ged commented Nov 18, 2019

Uh oh!

larskanis commented Nov 18, 2019

Uh oh!

ged commented Nov 18, 2019

Uh oh!

larskanis commented Nov 19, 2019

Uh oh!

ged commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ged commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

larskanis commented Nov 10, 2019 •

edited

Loading