Skip to content

Speed up symbol equality comparison - #6191

Merged
matz merged 3 commits into
mruby:masterfrom
leviongit:sym-eql
Apr 3, 2024
Merged

Speed up symbol equality comparison#6191
matz merged 3 commits into
mruby:masterfrom
leviongit:sym-eql

Conversation

@leviongit

Copy link
Copy Markdown
Contributor

No description provided.

@leviongit
leviongit requested a review from matz as a code owner March 7, 2024 15:23
@github-actions github-actions Bot added the core label Mar 7, 2024
Comment thread src/vm.c Outdated
if (mrb_obj_eq(mrb, regs[a], regs[a+1])) {
SET_TRUE_VALUE(regs[a]);
}
else if (mrb_type(regs[a]) == MRB_TT_SYMBOL) {

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.

Thank you for the pull-request. This is a nice idea. But if the condition is mrb_immediate_p(regs[a]), equalify comparison will be (slightly) faster not only for symbols, but other immediate values such as integers and float numbers.

@leviongit leviongit Mar 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this would unfortunately break some test cases, as it means that some (e.g. Integer#==(Rational)) equality checks are never performed.

Comment thread src/symbol.c Outdated
mrb_define_method_id(mrb, sym, MRB_SYM(to_sym), mrb_obj_itself, MRB_ARGS_NONE()); /* 15.2.11.3.4 */
mrb_define_method_id(mrb, sym, MRB_SYM(inspect), sym_inspect, MRB_ARGS_NONE()); /* 15.2.11.3.5(x) */
mrb_define_method_id(mrb, sym, MRB_OPSYM(cmp), sym_cmp, MRB_ARGS_REQ(1));
mrb_define_method_id(mrb, sym, MRB_OPSYM(eq), sym_eq, MRB_ARGS_REQ(1));

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.

I don't think this sym_eq change would improve the performance, since mrb_obj_equal_m (that defines Object#==) has exactly the same implementation. Should be removed from the PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Symbol inherits the == method from Comparable which does a lexical comparison. a new commit will override == to use mrb_obj_equal_m. (or should i make a separate pr for this?)

@leviongit
leviongit requested a review from matz March 9, 2024 13:08
@matz
matz merged commit c42f4e1 into mruby:master Apr 3, 2024
@leviongit
leviongit deleted the sym-eql branch January 26, 2025 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants