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

generate JIT argument type information for methods #8040

Merged
merged 2 commits into from Oct 27, 2015

Conversation

@froydnj
Copy link
Contributor

froydnj commented Oct 15, 2015

This enhances CodegenRust.py to output JSTypedMethodJitInfo structures where appropriate. This brings a notable speedup to tests like Dromaeo's dom-attr/getAttribute, which improves by several orders of magnitude with these patches applied.

If there are tricks for addressing the XXX comments, I would appreciate hearing them.

I think this addresses all of #6904.

Review on Reviewable

@frewsxcv
Copy link
Member

frewsxcv commented Oct 15, 2015

In case you missed it there are merge conflicts.

@jdm jdm added the S-needs-rebase label Oct 15, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 16, 2015

Maybe ArgType should use bitflags?

@froydnj froydnj force-pushed the froydnj:codegen-arg-info branch from 0e983e7 to de4a260 Oct 16, 2015
@jdm jdm added S-fails-tidy and removed S-needs-rebase labels Oct 16, 2015
@froydnj froydnj force-pushed the froydnj:codegen-arg-info branch from de4a260 to 216264c Oct 16, 2015
@nox nox removed the S-fails-tidy label Oct 19, 2015
@froydnj
Copy link
Contributor Author

froydnj commented Oct 20, 2015

r? @jdm

@jdm
Copy link
Member

jdm commented Oct 26, 2015

-S-awaiting-review +S-needs-code-changes
Ideally we could make ArgType use bitflags (although I don't know if those would work in const values right now), but that would require hand-modifying the rust-mozjs bindings right now. I'd rather use our knowledge of the types involved to make this work correctly.


Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/script/dom/bindings/codegen/CodegenRust.py, line 3097 [r2] (raw file):
If we make this i32 instead of ArgType, we can use | willy-nilly if we cast our ArgType values to i32.


components/script/dom/bindings/codegen/CodegenRust.py, line 3104 [r2] (raw file):
Then this becomes &${argTypes} as *const _ as *const ArgType.


Comments from the review on Reviewable.io

froydnj added 2 commits Oct 16, 2015
These are copied directly from Gecko's Codegen.py, with two changes:

- We need to use ArgType:: to qualify the enums rather than plain
  JSJitInfo::

- Given Rust's stronger notion of enums, we need to treat everything as an
  i32 so we can bitwise-or things together.
@froydnj froydnj force-pushed the froydnj:codegen-arg-info branch from 216264c to 4f649ac Oct 27, 2015
@jdm
Copy link
Member

jdm commented Oct 27, 2015

@bors-servo: r+


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

📌 Commit 4f649ac has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

Testing commit 4f649ac with merge d1295e9...

bors-servo added a commit that referenced this pull request Oct 27, 2015
generate JIT argument type information for methods

This enhances `CodegenRust.py` to output `JSTypedMethodJitInfo` structures where appropriate.  This brings a notable speedup to tests like Dromaeo's `dom-attr/getAttribute`, which improves by several orders of magnitude with these patches applied.

If there are tricks for addressing the XXX comments, I would appreciate hearing them.

I think this addresses all of #6904.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8040)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

@bors-servo bors-servo merged commit 4f649ac into servo:master Oct 27, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.