Skip to content

Add in the optimization to check the first/least timer in the queue before iterating#27841

Closed
ejanzer wants to merge 1 commit into
facebook:masterfrom
ejanzer:export-D19522346
Closed

Add in the optimization to check the first/least timer in the queue before iterating#27841
ejanzer wants to merge 1 commit into
facebook:masterfrom
ejanzer:export-D19522346

Conversation

@ejanzer
Copy link
Copy Markdown

@ejanzer ejanzer commented Jan 22, 2020

Summary

Follow up from #27539 - adding back in the optimization that Detox has in TimersIdlingResource to avoid iterating over the entire timers queue if the next timer is already within the specified range.

Changelog

[Internal]

Test Plan

TimingModuleTest

Differential Revision: D19522346

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner RN Team labels Jan 22, 2020
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D19522346

@ejanzer ejanzer assigned ejanzer and unassigned ejanzer Jan 22, 2020
@pull-bot
Copy link
Copy Markdown

pull-bot commented Jan 22, 2020

Messages
📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

DetailsCATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 0a514a8

@ejanzer
Copy link
Copy Markdown
Author

ejanzer commented Jan 22, 2020

cc @d4vidi

@d4vidi
Copy link
Copy Markdown
Contributor

d4vidi commented Jan 23, 2020

LGTM, except for maybe these concerns:

  1. Some (very minimal) code duplication (timer "active" or not check)
  2. Not sure what the policy here is but I'd add test coverage using unit tests

@ejanzer
Copy link
Copy Markdown
Author

ejanzer commented Jan 29, 2020

@d4vidi Agree about the repetition, I'll make a change. I did actually add unit tests for this function in the previous PR: https://github.com/facebook/react-native/blob/master/ReactAndroid/src/test/java/com/facebook/react/modules/timing/TimingModuleTest.java#L259

I haven't added any tests explicitly for this optimization, because that feels like an implementation detail that probably doesn't make sense to test (the functionality hasn't changed).

…efore iterating (facebook#27841)

Summary:
Pull Request resolved: facebook#27841

Follow up from facebook#27539 - adding back in the optimization that Detox has in TimersIdlingResource to avoid iterating over the entire timers queue if the next timer is already within the specified range.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D19522346

fbshipit-source-id: 259587031fef2971f84027df97d240ae85a401ed
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D19522346

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @ejanzer in 8f5779c.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 29, 2020
osdnk pushed a commit to osdnk/react-native that referenced this pull request Mar 9, 2020
…efore iterating (facebook#27841)

Summary:
Pull Request resolved: facebook#27841

Follow up from facebook#27539 - adding back in the optimization that Detox has in TimersIdlingResource to avoid iterating over the entire timers queue if the next timer is already within the specified range.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D19522346

fbshipit-source-id: 32609f434d1ca575a5a49ad630e288c43fa90864
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. Merged This PR has been merged. p: Facebook Partner: Facebook Partner RN Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants