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

Remove failing assert in JSC Options.cpp. #7340

Conversation

@justinmichaud justinmichaud requested a review from a team as a code owner December 8, 2022 21:04
@justinmichaud justinmichaud self-assigned this Dec 8, 2022
@justinmichaud justinmichaud added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Dec 8, 2022
@@ -747,7 +747,7 @@ void Options::notifyOptionsChanged()

// Do range checks where needed and make corrections to the options:
ASSERT(Options::thresholdForOptimizeAfterLongWarmUp() >= Options::thresholdForOptimizeAfterWarmUp());
ASSERT(Options::thresholdForOptimizeAfterWarmUp() >= Options::thresholdForOptimizeSoon());
// ASSERT(Options::thresholdForOptimizeAfterWarmUp() >= Options::thresholdForOptimizeSoon());

Choose a reason for hiding this comment

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

Why comment it out instead of removing it? Also, in the commit msg, it would be good to explain why this should be removed.

@justinmichaud justinmichaud force-pushed the eng/Remove-failing-assert-in-JSC-Options-cpp branch from abfb2f3 to 5809f5c Compare December 8, 2022 21:13
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

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

r=me

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Dec 8, 2022
https://bugs.webkit.org/show_bug.cgi?id=248971

Reviewed by Yusuke Suzuki.

We have tests that test this configuration, like
stress/ftl-osr-entry-order-reverse.js. We should
temporarily remove this assertion so that debug
tests pass.

* Source/JavaScriptCore/runtime/Options.cpp:
(JSC::Options::notifyOptionsChanged):

Canonical link: https://commits.webkit.org/257586@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/Remove-failing-assert-in-JSC-Options-cpp branch from 5809f5c to 0893db1 Compare December 8, 2022 21:32
@webkit-early-warning-system webkit-early-warning-system merged commit 0893db1 into WebKit:main Dec 8, 2022
@webkit-commit-queue
Copy link
Collaborator

Committed 257586@main (0893db1): https://commits.webkit.org/257586@main

Reviewed commits have been landed. Closing PR #7340 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Dec 8, 2022
@justinmichaud justinmichaud deleted the eng/Remove-failing-assert-in-JSC-Options-cpp branch March 20, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
5 participants