Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New attribute trace_equivalent #2143

Closed
wants to merge 1 commit into from

Conversation

shyouhei
Copy link
Member

@shyouhei shyouhei commented Apr 23, 2019

Some instructions are spacial cases for another ones. These instructions need not preserve their trace counterparts. By reducing those unnecessary trace instructions we can strip binary size of vm_exec_core from 25,759 bytes to 24,924 bytes on my machine.

Yes, this changeset slows traces down a bit. But is that a problem?

Some instructions are spacial cases for another ones.  These
instructions need not preserve their trace counterparts.  By reducing
those unnecessary trace instructions we can strip binary size of
vm_exec_core from 25,759 bytes to 24,924 bytes on my machine.

Yes, this changeset slows traces down a bit.  But is that a problem?
@shyouhei shyouhei requested review from ko1 and k0kubun April 23, 2019 02:35
@shyouhei
Copy link
Member Author

This is a part of #2100.

@ko1
Copy link
Contributor

ko1 commented Apr 23, 2019

These instructions need not preserve their trace counterparts

could you explain more with examples?

@shyouhei
Copy link
Member Author

These instructions need not preserve their trace counterparts

could you explain more with examples?

For instance opt_plus has trace_opt_plus now. But trace_opt_send_without_block works 100% well when tracing opt_plus. So we are deleting trace_opt_plus and use trace_opt_send_without_block instead.

@k0kubun
Copy link
Member

k0kubun commented Apr 23, 2019

I don't have a strong opinion on which we should prioritize, a binary size or performance, but as long as we're making some decision on trade-off, please share the benchmark results just in case.


Possibly we may want to reuse the attr for

% send_compatible_opt_insns = RubyVM::BareInstructions.to_a.select do |insn|
% insn.name.start_with?('opt_') && opt_send_without_block.opes == insn.opes &&
% insn.expr.expr.lines.any? { |l| l.match(/\A\s+CALL_SIMPLE_METHOD\(\);\s+\z/) }
% end.map(&:name)
. If the information is not exactly the same as what you want to achieve, never mind.

@shyouhei
Copy link
Member Author

@k0kubun I don't know any benchmark instance that focuses on tracing performance. Do you have one? This changeset obviously does not affect normal runs.

@ko1
Copy link
Contributor

ko1 commented Apr 24, 2019

For instance opt_plus has trace_opt_plus now. But trace_opt_send_without_block works 100% well when tracing opt_plus. So we are deleting trace_opt_plus and use trace_opt_send_without_block instead.

How to replace opt_plus to trace_opt_send_without_block ? Or make trace_opt_plus insn and it goto trace_opt_send_without_block impl immediately?

@k0kubun I don't know any benchmark instance that focuses on tracing performance. Do you have one? This changeset obviously does not affect normal runs.

Now, if we replace trace_xxx insn, then we don't restore to xxx insn even if trace is disabled. It can cause performance downgrade.

@shyouhei
Copy link
Member Author

How to replace opt_plus to trace_opt_send_without_block ? Or make trace_opt_plus insn and it goto trace_opt_send_without_block impl immediately?

Look at the patch :) That's the same as before. We only change how encoded_insn_data is constructed.

Now, if we replace trace_xxx insn, then we don't restore to xxx insn even if trace is disabled. It can cause performance downgrade.

Yes. That's what I refer in "is that a problem?" in the original comment. Can we measure the magnitude somehow?

@shyouhei
Copy link
Member Author

#1970 can be something related to this.

@ruby ruby deleted a comment from softagram-bot Apr 25, 2019
@k0kubun k0kubun changed the base branch from trunk to master August 15, 2019 17:17
@shyouhei shyouhei closed this Feb 6, 2020
@shyouhei shyouhei deleted the compress_trace_insns branch February 6, 2020 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants