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

[Scheduler] Prevent event log from growing unbounded #16781

Merged
merged 1 commit into from
Sep 13, 2019

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Sep 13, 2019

If a Scheduler profile runs without stopping, the event log will grow unbounded. Eventually it will run out of memory and the VM will throw an error.

To prevent this from happening, let's automatically stop the profiler once the log exceeds a certain limit. We'll also print a warning with advice to call stopLoggingProfilingEvents explicitly.

If a Scheduler profile runs without stopping, the event log will grow
unbounded. Eventually it will run out of memory and the VM will throw
an error.

To prevent this from happening, let's automatically stop the profiler
once the log exceeds a certain limit. We'll also print a warning with
advice to call `stopLoggingProfilingEvents` explicitly.
);
eventLogSize *= 2;
if (eventLogSize > MAX_EVENT_LOG_SIZE) {
console.error(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This message will increase the size of the profiling build. Seems fine? Idk.

: MAX_TEST_ITERATIONS;
const maxIterations = t.logicalExpression(
'||',
t.memberExpression(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gaearon Might want to review the changes I made in here. I wanted to increase the limit for a specific test so I could trigger the memory error.

@acdlite acdlite force-pushed the scheduler-profiler-memory branch 2 times, most recently from 8fe678d to 1f55c5a Compare September 13, 2019 22:25
@sizebot
Copy link

sizebot commented Sep 13, 2019

Details of bundled changes.

Comparing: 87eaa90...1f55c5a

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.3% +0.4% 117.47 KB 117.79 KB 29.82 KB 29.95 KB UMD_DEV
react.profiling.min.js +0.7% +1.6% 15.83 KB 15.94 KB 5.9 KB 5.99 KB UMD_PROFILING
react.development.js 0.0% -0.0% 73.12 KB 73.12 KB 19.25 KB 19.25 KB NODE_DEV
React-prod.js 0.0% 0.0% 17.41 KB 17.41 KB 4.55 KB 4.56 KB FB_WWW_PROD
React-profiling.js 0.0% 0.0% 17.41 KB 17.41 KB 4.55 KB 4.56 KB FB_WWW_PROFILING

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler-unstable_mock.development.js +1.4% +2.9% 22.12 KB 22.43 KB 5.13 KB 5.28 KB UMD_DEV
scheduler-tracing.profiling.min.js 0.0% +0.1% 3.25 KB 3.25 KB 991 B 992 B NODE_PROFILING
scheduler-unstable_mock.production.min.js 0.0% -0.0% 4.73 KB 4.73 KB 1.98 KB 1.98 KB UMD_PROD
Scheduler-dev.js +1.1% +1.8% 29.7 KB 30.03 KB 7.54 KB 7.67 KB FB_WWW_DEV
Scheduler-profiling.js +1.2% +2.1% 16.84 KB 17.04 KB 3.85 KB 3.93 KB FB_WWW_PROFILING
scheduler-tracing.development.js 0.0% 0.0% 11.72 KB 11.72 KB 3.03 KB 3.03 KB NODE_DEV
scheduler-tracing.production.min.js 0.0% -0.5% 728 B 728 B 384 B 382 B NODE_PROD
scheduler-unstable_mock.development.js +1.4% +2.9% 21.93 KB 22.24 KB 5.07 KB 5.22 KB NODE_DEV
SchedulerMock-dev.js +1.5% +2.8% 22.33 KB 22.66 KB 5.17 KB 5.31 KB FB_WWW_DEV
scheduler.development.js +1.1% +1.8% 29.16 KB 29.47 KB 7.43 KB 7.57 KB NODE_DEV

Generated by 🚫 dangerJS against 1f55c5a

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

I don't like this test. :) It's unnecessarily slow and binds us to this particular variant of infinite loop tracking.

@acdlite
Copy link
Collaborator Author

acdlite commented Sep 13, 2019

Ok I'm going to land as-is though because I don't have any better ideas for how to test it right now, and I'd rather have a slow test than no test

@acdlite acdlite merged commit 45898d0 into facebook:master Sep 13, 2019
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.

4 participants