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

Flip expiration times #13912

Merged
merged 3 commits into from
Oct 30, 2018
Merged

Flip expiration times #13912

merged 3 commits into from
Oct 30, 2018

Conversation

sophiebits
Copy link
Collaborator

Now:

0 - NoWork
1 - Never
...
big - low pri things
even bigger - hi pri things
max 31-bit int - Sync

Per @sebmarkbage in #13904.

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2018

These all correspond to millisecond timestamps... and now they’re reversed?

If our model weren’t so tied to a timeline and these were merely relative priorities, I would like this. But since they represent deadlines I find it confusing.

@sizebot
Copy link

sizebot commented Oct 21, 2018

ReactDOM: size: -0.2%, gzip: -0.2%

Details of bundled changes.

Comparing: bf9fadf...29e1b1f

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js -0.2% -0.1% 705.01 KB 703.61 KB 163.13 KB 162.94 KB UMD_DEV
react-dom.production.min.js -0.2% -0.2% 102.57 KB 102.41 KB 33.54 KB 33.48 KB UMD_PROD
react-dom.development.js -0.2% -0.1% 700.32 KB 698.92 KB 161.75 KB 161.57 KB NODE_DEV
react-dom.production.min.js -0.2% -0.3% 102.66 KB 102.5 KB 33.07 KB 32.99 KB NODE_PROD
ReactDOM-dev.js -0.3% -0.2% 719.2 KB 717.18 KB 162.66 KB 162.4 KB FB_WWW_DEV
ReactDOM-prod.js -0.4% -0.3% 311.36 KB 310.07 KB 57.41 KB 57.23 KB FB_WWW_PROD
react-dom.profiling.min.js -0.2% -0.3% 105.07 KB 104.87 KB 33.41 KB 33.32 KB NODE_PROFILING
ReactDOM-profiling.js -0.5% -0.4% 316.02 KB 314.4 KB 58.38 KB 58.15 KB FB_WWW_PROFILING
react-dom.profiling.min.js -0.2% -0.3% 104.94 KB 104.74 KB 33.93 KB 33.82 KB UMD_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js -0.3% -0.2% 492.21 KB 490.74 KB 108.88 KB 108.67 KB UMD_DEV
react-art.production.min.js -0.2% -0.1% 94.52 KB 94.29 KB 29.13 KB 29.11 KB UMD_PROD
react-art.development.js -0.3% -0.2% 423.98 KB 422.52 KB 91.85 KB 91.66 KB NODE_DEV
react-art.production.min.js -0.4% -0.4% 59.56 KB 59.33 KB 18.46 KB 18.38 KB NODE_PROD
ReactART-dev.js -0.5% -0.3% 429.22 KB 427.12 KB 90.5 KB 90.21 KB FB_WWW_DEV
ReactART-prod.js -0.7% -0.7% 184.53 KB 183.22 KB 31.59 KB 31.37 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js -0.3% -0.2% 436.62 KB 435.16 KB 94.53 KB 94.33 KB UMD_DEV
react-test-renderer.production.min.js -0.4% -0.3% 60.74 KB 60.52 KB 18.85 KB 18.8 KB UMD_PROD
react-test-renderer.development.js -0.3% -0.2% 431.84 KB 430.38 KB 93.39 KB 93.18 KB NODE_DEV
react-test-renderer.production.min.js -0.4% -0.4% 60.42 KB 60.21 KB 18.62 KB 18.55 KB NODE_PROD
ReactTestRenderer-dev.js -0.5% -0.3% 437.27 KB 435.17 KB 92.36 KB 92.08 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js -0.3% -0.2% 421.76 KB 420.36 KB 90.3 KB 90.1 KB NODE_DEV
react-reconciler.production.min.js -0.2% -0.4% 60.69 KB 60.55 KB 18.26 KB 18.18 KB NODE_PROD
react-reconciler-persistent.development.js -0.3% -0.2% 420.21 KB 418.81 KB 89.68 KB 89.48 KB NODE_DEV
react-reconciler-persistent.production.min.js -0.2% -0.4% 60.7 KB 60.56 KB 18.27 KB 18.18 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js -0.4% -0.3% 555.42 KB 553.4 KB 121.35 KB 121.03 KB RN_FB_DEV
ReactNativeRenderer-prod.js -0.5% -0.4% 239.32 KB 238.11 KB 42.09 KB 41.9 KB RN_FB_PROD
ReactNativeRenderer-dev.js -0.4% -0.3% 555.1 KB 553.07 KB 121.26 KB 120.93 KB RN_OSS_DEV
ReactNativeRenderer-prod.js -0.5% -0.4% 239.34 KB 238.13 KB 42.08 KB 41.9 KB RN_OSS_PROD
ReactFabric-dev.js -0.4% -0.3% 545.61 KB 543.58 KB 118.88 KB 118.57 KB RN_FB_DEV
ReactFabric-prod.js -0.5% -0.4% 234.21 KB 233 KB 40.8 KB 40.63 KB RN_FB_PROD
ReactFabric-dev.js -0.4% -0.3% 545.64 KB 543.62 KB 118.89 KB 118.59 KB RN_OSS_DEV
ReactFabric-prod.js -0.5% -0.4% 234.25 KB 233.03 KB 40.81 KB 40.64 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js -0.6% -0.5% 245.29 KB 243.9 KB 43.36 KB 43.13 KB RN_OSS_PROFILING
ReactFabric-profiling.js -0.6% -0.5% 239.19 KB 237.65 KB 42.13 KB 41.9 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js -0.6% -0.5% 245.27 KB 243.88 KB 43.36 KB 43.13 KB RN_FB_PROFILING
ReactFabric-profiling.js -0.6% -0.5% 239.15 KB 237.61 KB 42.12 KB 41.89 KB RN_FB_PROFILING

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@sophiebits
Copy link
Collaborator Author

You win some, you lose some. Just pretend you're looking in a mirror.

@sophiebits
Copy link
Collaborator Author

Want me to make them all negative? 🙄

min 31-bit int - Sync
large negative - hi pri things
less-large negative - low pri things
...
–1 - Never
0 - NoWork

Then 0 is in the right place and it goes in the right direction…

@sebmarkbage
Copy link
Collaborator

I think we can find a mental model where this makes sense. E.g. a count down until the app stops working (NoWork).

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2018

I’m cool with this if we rename the field, too. Like timeRemaining or something with “priority” in it.

(childChildExpirationTime !== NoWork &&
childChildExpirationTime < newChildExpirationTime)
) {
if (childChildExpirationTime > newChildExpirationTime) {
newChildExpirationTime = childChildExpirationTime;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point we could even go so far as to do

newChildExpirationTime = Math.max(
  newChildExpirationTime,
  child.expirationTime,
  child.childExpirationTime,
);

so simple… wow

doge meme

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

@sophiebits
Copy link
Collaborator Author

I kinda like the negative clock (that becomes less negative over time) actually… what about you two?

@sebmarkbage
Copy link
Collaborator

Nah. Negative is weird. That's not Rust/WASM/et al friendly since we'd want the unsigned form there.

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2018

If we didn’t already have something called “priority” I would say let’s rename it to that and merge your PR as is.

I don’t like the “expirationTime” naming regardless because it only acts like an expiration time when we schedule the callback with Scheduler. Everywhere else it acts like a relative priority.

Precedence? Urgency? Rank? (I’m just reading the thesaurus)

@sophiebits
Copy link
Collaborator Author

"Threshold" maybe.

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2018

Importance?

@sebmarkbage
Copy link
Collaborator

Note that we also plan to add another public priority API which is not related to time (the priority of fallbacks returning to their unsuspended state).

@gaearon
Copy link
Collaborator

gaearon commented Oct 21, 2018

Urgency?

@sophiebits
Copy link
Collaborator Author

We're hopeless.

@gaearon
Copy link
Collaborator

gaearon commented Oct 21, 2018

Oh I missed @acdlite already suggested urgency.

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2018

@sebastian yeah and I also don’t think we understand the relationship between the two concepts yet, either.

@sophiebits
Copy link
Collaborator Author

restlessness

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Oct 21, 2018

If we didn’t already have something called “priority” I would say let’s rename it to that and merge your PR as is.
I don’t like the “expirationTime” naming regardless because it only acts like an expiration time when we schedule the callback with Scheduler. Everywhere else it acts like a relative priority.

Agree, we use the priority name before but then use expirationTime to abstract too many things for processing starvation and competition better. But just from a developer perspective, the expirationTime is confused and hard to understand(lower means higher, higher means lower), and when you compare it to some names like xxxxWork, it's weird(a time to compare a work, what's that, oh, they are just numbers...)

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rename the fields later once we think of something good

```
codemod --accept-all -d packages -i '((?:expiration|suspended|pending|current|remaining|absolute|renderer|pinged|completed)time(?:(?:before|after)commit)? )<' '\1dnkvjchbdddtngeurvlbvbktrilldeeu'
codemod --accept-all -d packages -i '((?:expiration|suspended|pending|current|remaining|absolute|renderer|pinged|completed)time(?:(?:before|after)commit)? )>' '\1<'
codemod --accept-all -d packages -i '((?:expiration|suspended|pending|current|remaining|absolute|renderer|pinged|completed)time(?:(?:before|after)commit)? )dnkvjchbdddtngeurvlbvbktrilldeeu' '\1>'
git checkout packages/scheduler/src/Scheduler.js
```
Ran these, then prettier, then these again, then prettier again.

```
codemod --accept-all -d packages -m '\(\s*([a-zA-Z.]+) === NoWork\s*\|\|\s*\1 (<=?) ([a-zA-Z.]+)\s*\)' '(\1 \2 \3)'
codemod --accept-all -d packages -m '\(\s*([a-zA-Z.]+) === NoWork\s*\|\|\s*([a-zA-Z.]+) (>=?) \1\s*\)' '(\2 \3 \1)'

codemod --accept-all -d packages -m '(?<![.\w])([a-zA-Z.]+) !== NoWork\s*&&\s*\1 (>) ([a-zA-Z.]+)' '\1 \2 \3'
codemod --accept-all -d packages -m '(?<![.\w])([a-zA-Z.]+) !== NoWork\s*&&\s*([a-zA-Z.]+) (<) \1' '\2 \3 \1'

codemod --accept-all -d packages -m '(?<![.\w])([a-zA-Z.]+) === NoWork\s*\|\|\s*\1 (<=?) ([a-zA-Z.]+)\s*\?\s' '\1 \2 \3 ? '
codemod --accept-all -d packages -m '(?<![.\w])([a-zA-Z.]+) === NoWork\s*\|\|\s*([a-zA-Z.]+) (>=?) \1\s*\?\s' '\2 \3 \1 ? '
```
@sophiebits sophiebits merged commit 169f935 into facebook:master Oct 30, 2018
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
See facebook#13912 commit messages for how this was done.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants