Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Back porting Denis Pravdin changes to CPU Profiling code. #58

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 29, 2014

This is a back port of changes that Denis @dp7 are currently pushing to the upstream. This change ensures blink tests are passing as well as crosswalk ones. Also it fixes 24 debug v8 tests currently failing with segfault.

Please note that although almost the same changes are being pushed to the upstream we still need this separate patch because it may take significant amount of time to put the changes into upstream and then back to crosswalk branch.

@rakuco
Copy link

rakuco commented Oct 29, 2014

Thanks for the patch. There are some points we need to clarify to proceed:

  • Why is this backported only to crosswalk-9 and not master? Assuming Denis's changes make it into Chromium M40, there will be a whole Crosswalk release cycle (Crosswalk 10, with Chromium M39) without those fixes.
  • Why are the changes "almost" the same being upstreamed and not exactly the same ones being upstreamed? You could even cherry-pick the changes that have already been committed upstream.
  • The commit message is too vague. It just says something similar to "backport some changes to make sure test are passing". It does not say which changes are being backported from where, nor which tests are failing. In this regard, the pull request message is much more descriptive, even though it does not address all these points either.

@ghost
Copy link
Author

ghost commented Oct 29, 2014

@rakuco, thank you for the feedback. I will try to answer your questions.

  • This is being backported to crosswalk-9 because it is the branch xwalk currently is built with. First I want to push it to crosswalk-9 (if it is not too late yet) and then to master and to crosswalk-10.
  • The changes are almost the same because I could not apply Denis's patch as is. I had to apply it manually line by line. Some lines were skipped and some lines were from chromium bleeding edge branch and so on. I will see if I can cherry-pick needed changes automatically.
  • Sorry for the vague message. The changes intended to fix issues with incorrect line info reported when "turbo fan" JIT compiler is used. They also fix the following debug test failures (the list is not complete):

mjsunit/mul-exhaustive-part10
mjsunit/harmony/proxies-example-membrane
mjsunit/math-round
cctest/test-api/InterceptorCallICFastApi_Simple...
cctest/test-heap-profiler/HeapSnapshotJSONSeria...
mjsunit/regress/regress-747
cctest/test-api/ScavengeExternalAsciiString
cctest/test-heap/Regress1465
cctest/test-api/IndependentHandleRevival
mjsunit/harmony/regress/regress-lookup-transiti...

@rakuco
Copy link

rakuco commented Oct 29, 2014

This is being backported to crosswalk-9 because it is the branch xwalk currently is built with. First I want to push it to crosswalk-9 (if it is not too late yet) and then to master and to crosswalk-10.

That's the opposite of our development model (which is also Chromium's): everything lands in trunk/master/bleeding_edge first, and is only backported to more stable branches after it has been minimally tested in master and it has been verified it did not break anything.

The changes are almost the same because I could not apply Denis's patch as is. I had to apply it manually line by line. Some lines were skipped and some lines were from chromium bleeding edge branch and so on. I will see if I can cherry-pick needed changes automatically.

OK. In any case, please include a reference to the upstream commit numbers (or code review links if the patches haven't been committed yet) in the PR/commit messages.

Sorry for the vague message. The changes intended to fix issues with incorrect line info reported when "turbo fan" JIT compiler is used. They also fix the following debug test failures (the list is not complete):

Right, please include this information in your pull request and commit messages.

@ghost
Copy link
Author

ghost commented Oct 29, 2014

Ok. Now I think I got it. I am going to discard this PR and first back port the changes in to master branch. If they look OK I back port it from master to crosswalk-9 and crosswalk-10. Is that correct?

@darktears
Copy link

@eleskine that's correct, target master, land it there, roll the DEPS.xwalk, look the bots and the QA reports on the ML, and then backport to crosswalk 9 and 10.

@ghost
Copy link
Author

ghost commented Oct 29, 2014

Thank you @rakuco, @darktears. I will follow the procedure. I am sorry for wasting your time

@ghost ghost closed this Oct 29, 2014
@rakuco
Copy link

rakuco commented Oct 29, 2014

It wasn't a waste of time, it's good to clear any misconceptions as early as possible :-)

mrunalk pushed a commit that referenced this pull request Jun 11, 2016
Merged ca266e7

[arm] Make CEntryStub's handling of triple return values more robust.

BUG=chromium:611885
LOG=N
[email protected]

Review URL: https://codereview.chromium.org/2010633003 .

Cr-Commit-Position: refs/branch-heads/5.1@{#58}
Cr-Branched-From: 167dc63-refs/heads/5.1.281@{#1}
Cr-Branched-From: 03953f5-refs/heads/master@{#35282}
rakuco pushed a commit that referenced this pull request Sep 21, 2016
Cr-Commit-Position: refs/branch-heads/5.3@{#58}
Cr-Branched-From: 820a23a-refs/heads/5.3.332@{#2}
Cr-Branched-From: 37538cb-refs/heads/master@{#37308}
rakuco pushed a commit that referenced this pull request Oct 17, 2016
Cr-Commit-Position: refs/branch-heads/5.4@{#58}
Cr-Branched-From: 5ce2827-refs/heads/5.4.500@{#2}
Cr-Branched-From: ad07b49-refs/heads/master@{#38841}
imreotto pushed a commit to tenta-browser/v8-crosswalk that referenced this pull request Sep 4, 2017
Cr-Commit-Position: refs/branch-heads/5.9@{crosswalk-project#58}
Cr-Branched-From: fe9bb7e-refs/heads/5.9.211@{crosswalk-project#1}
Cr-Branched-From: 70ad237-refs/heads/master@{#44591}
asifhisam pushed a commit to asifhisam/v8-crosswalk that referenced this pull request Sep 9, 2019
Merged: [parser] Fix crash when lazy arrow func params contain destructuring assignments.
Revision: bc39a51

Merged: [parser] don't rewrite destructuring assignments in params for lazy top level arrow functions
Revision: 5f782db

BUG=chromium:704811,chromium:706234,chromium:706761,v8:6182
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true

Change-Id: If5c04c3b9f6ac9c6879052b6a34446f895624200
Reviewed-on: https://chromium-review.googlesource.com/474746
Reviewed-by: Michael Achenbach <[email protected]>
Cr-Commit-Position: refs/branch-heads/5.8@{crosswalk-project#58}
Cr-Branched-From: eda659c-refs/heads/5.8.283@{crosswalk-project#1}
Cr-Branched-From: 4310cd0-refs/heads/master@{#43429}
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants