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

Android: Decrease cost of reflection #11204

Closed

Conversation

rigdern
Copy link
Contributor

@rigdern rigdern commented Nov 29, 2016

This change suppresses access checking during reflection which makes reflection faster by decreasing its overhead.

Test plan (required)

My team uses this change in our app.

Adam Comella
Microsoft Corp.

This change suppresses accessing checking during reflection which makes reflection faster by decreasing its overhead.
@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @astreet and @lexs to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Nov 29, 2016
@astreet
Copy link
Contributor

astreet commented Nov 30, 2016

@facebook-github-bot import

Going to perf test this internally -- did you all measure any perf wins with this?

@astreet
Copy link
Contributor

astreet commented Nov 30, 2016

(I mean, this is probably strictly better than what we currently have so I'll land it anyway :) )

@facebook-github-bot
Copy link
Contributor

@astreet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@rigdern
Copy link
Contributor Author

rigdern commented Nov 30, 2016

@astreet One of my teammates commented on the performance improvement we saw here:

dryganets wrote:

I was optimizing particular flow in our application. We have VirtualListView component and I was testing scrolling inside. In addition to that we have a lot of ongoing activity through the bridge. Mostly timers.

I was using Android tracing to profile performance.

After the change reflection takes 15% of CPU. Before around 60% if I remember correctly.

I also had a synthetic test for reflection only. I was able to made the same count of calls in half of the time with property set.

For sure it would depend on your exact scenario.
For simple calls it has bigger impact.

AaaChiuuu ran a benchmark but said he didn't see any improvement:

AaaChiuuu wrote:

I ran 71c5bb6 on an internal RN cold start benchmark which has about 1000 method calls via reflection on the startup path of a fairly complex UI and it showed no statistically significant gains. I'm curious to understand how this was profiled to show an 80% win. Seems like there's a chance this win is far less than 80% on reflection calls or the cost of reflection is almost nominal compared to other costs.

@rigdern rigdern deleted the rigdern/androidDecreaseReflectionCost branch December 5, 2016 21:30
robclouth pushed a commit to robclouth/react-native that referenced this pull request Dec 7, 2016
Summary:
This change suppresses access checking during reflection which makes reflection faster by decreasing its overhead.

**Test plan (required)**

My team uses this change in our app.

Adam Comella
Microsoft Corp.
Closes facebook#11204

Differential Revision: D4250790

Pulled By: astreet

fbshipit-source-id: 0ee2f40dcadccc695980fcae14fafe1050acb52f
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
This change suppresses access checking during reflection which makes reflection faster by decreasing its overhead.

**Test plan (required)**

My team uses this change in our app.

Adam Comella
Microsoft Corp.
Closes facebook#11204

Differential Revision: D4250790

Pulled By: astreet

fbshipit-source-id: 0ee2f40dcadccc695980fcae14fafe1050acb52f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants