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

Don't emit duplicate source locations #917

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Contributor

Requires a little extra bookkeeping but there are two good reasons to accept that overhead:

  1. OP_source_loc is pretty big at 9 bytes so duplicates bloat the bytecode considerably

  2. An OP_source_loc opcode can throw off the peephole optimizer if it's between two opcodes the optimizer is trying to match; removing duplicates reduces the chance of that happening

Fixes: #916

Requires a little extra bookkeeping but there are two good reasons to
accept that overhead:

1. OP_source_loc is pretty big at 9 bytes so duplicates bloat the
   bytecode considerably

2. An OP_source_loc opcode can throw off the peephole optimizer if it's
   between two opcodes the optimizer is trying to match; removing
   duplicates reduces the chance of that happening

Fixes: quickjs-ng#916
@bnoordhuis
Copy link
Contributor Author

  1. An OP_source_loc opcode can throw off the peephole optimizer if it's between two opcodes the optimizer is trying to match; removing duplicates reduces the chance of that happening

Looking at code_match() more closely, it seems smart enough to skip them. That leaves argument (1) and since they're filtered out in phase 3 anyway, maybe it's not worth the overhead.

@bnoordhuis
Copy link
Contributor Author

I used callgrind and massif and benchmarked this change rather extensively on a big source file and...the results are confusing:

  1. Total instruction count dropped by 0.001%, but
  2. Peak memory usage went up 0.001% - wut?

They're marginal differences but callgrind and massif are exact and repeated runs are indeed identical.

I'm somewhat at a loss to explain (2) so I'll go ahead and close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elide duplicate OP_source_loc opcodes
2 participants