Reland "[oilpan] Move incremental marking behind a build-time flag" [chromium/src : master]

0 views
Skip to first unread message

Michael Lippautz (Gerrit)

unread,
Dec 15, 2017, 5:53:37 AM12/15/17
to blink-...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-...@chromium.org, Jeremy Roman, Commit Bot, Kentaro Hara, Dirk Pranke, chromium...@chromium.org, Findit, Mads Ager

PTAL

I changed the public_dep for the unittests to just require the headers and not the full platform/heap. This seems to fix linking on windows. Delta: https://chromium-review.googlesource.com/c/chromium/src/+/828734/1..3

View Change

    To view, visit change 828734. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I56d11f001fc2717329b3caf0e815adac76b6ddb2
    Gerrit-Change-Number: 828734
    Gerrit-PatchSet: 3
    Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
    Gerrit-Reviewer: Commit Bot <commi...@chromium.org>
    Gerrit-Reviewer: Dirk Pranke <dpr...@chromium.org>
    Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
    Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
    Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
    Gerrit-CC: Mads Ager <ag...@chromium.org>
    Gerrit-Comment-Date: Fri, 15 Dec 2017 10:53:34 +0000
    Gerrit-HasComments: No
    Gerrit-HasLabels: No

    Kentaro Hara (Gerrit)

    unread,
    Dec 15, 2017, 6:10:05 AM12/15/17
    to Michael Lippautz, blink-...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-...@chromium.org, Commit Bot, Jeremy Roman, Dirk Pranke, chromium...@chromium.org, Findit, Mads Ager

    LGTM

    Patch set 3:Code-Review +1

    View Change

      To view, visit change 828734. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I56d11f001fc2717329b3caf0e815adac76b6ddb2
      Gerrit-Change-Number: 828734
      Gerrit-PatchSet: 3
      Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
      Gerrit-Reviewer: Commit Bot <commi...@chromium.org>
      Gerrit-Reviewer: Dirk Pranke <dpr...@chromium.org>
      Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
      Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
      Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
      Gerrit-CC: Mads Ager <ag...@chromium.org>
      Gerrit-Comment-Date: Fri, 15 Dec 2017 11:10:01 +0000
      Gerrit-HasComments: No
      Gerrit-HasLabels: Yes

      Michael Lippautz (Gerrit)

      unread,
      Dec 15, 2017, 6:29:55 AM12/15/17
      to blink-...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-...@chromium.org, Kentaro Hara, Commit Bot, Jeremy Roman, Dirk Pranke, chromium...@chromium.org, Findit, Mads Ager
      And now the chromeos builder is unhappy, see
      https://build.chromium.org/deprecated/tryserver.chromium.chromiumos/builders/chromeos-daisy-rel/builds/23331/steps/compile%20(with%20patch)/logs/stdio

      Anybody know what's going on?

      Gerrit-Comment-Date: Fri, 15 Dec 2017 11:29:49 +0000
      Gerrit-HasComments: No
      Gerrit-HasLabels: No

      Jeremy Roman (Gerrit)

      unread,
      Dec 15, 2017, 10:40:39 PM12/15/17
      to Michael Lippautz, blink-...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-...@chromium.org, Jeremy Roman, Commit Bot, Kentaro Hara, Dirk Pranke, chromium...@chromium.org, Findit, Mads Ager

      Patch Set 3:

      There's missing dependencies; the constituent targets of platform (in this case, instrumentation) need to have a transitive dependency on the build flag header, else it's not guaranteed to be generated in time for their compile steps.

      This is a bit of a mess in platform right now; aside from combining it into one target (which is the simplest approach, and I think where we used to be), we can mimic blink_core to some extent and address it.

      https://chromium-review.googlesource.com/c/chromium/src/+/830687 is a working prototype of that which fixes this issue in my local testing and passes the CQ. I can probably split off the generic refactoring of that into a separate CL to land in front of this one.

      View Change

        To view, visit change 828734. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-Project: chromium/src
        Gerrit-Branch: master
        Gerrit-MessageType: comment
        Gerrit-Change-Id: I56d11f001fc2717329b3caf0e815adac76b6ddb2
        Gerrit-Change-Number: 828734
        Gerrit-PatchSet: 3
        Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
        Gerrit-Reviewer: Commit Bot <commi...@chromium.org>
        Gerrit-Reviewer: Dirk Pranke <dpr...@chromium.org>
        Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
        Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
        Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
        Gerrit-CC: Mads Ager <ag...@chromium.org>
        Gerrit-Comment-Date: Sat, 16 Dec 2017 03:40:35 +0000
        Gerrit-HasComments: No
        Gerrit-HasLabels: No

        Michael Lippautz (Gerrit)

        unread,
        Dec 18, 2017, 2:33:28 AM12/18/17
        to blink-...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-...@chromium.org, Jeremy Roman, Commit Bot, Kentaro Hara, Dirk Pranke, chromium...@chromium.org, Findit, Mads Ager

        Patch Set 3:

        Patch Set 3:

        And now the chromeos builder is unhappy, see
        https://build.chromium.org/deprecated/tryserver.chromium.chromiumos/builders/chromeos-daisy-rel/builds/23331/steps/compile%20(with%20patch)/logs/stdio

        Anybody know what's going on?

        There's missing dependencies; the constituent targets of platform (in this case, instrumentation) need to have a transitive dependency on the build flag header, else it's not guaranteed to be generated in time for their compile steps.

        This is a bit of a mess in platform right now; aside from combining it into one target (which is the simplest approach, and I think where we used to be), we can mimic blink_core to some extent and address it.

        https://chromium-review.googlesource.com/c/chromium/src/+/830687 is a working prototype of that which fixes this issue in my local testing and passes the CQ. I can probably split off the generic refactoring of that into a separate CL to land in front of this one.

        Would be awesome if you could land a refactoring that cleans this up :)

        View Change

          To view, visit change 828734. To unsubscribe, or for help writing mail filters, visit settings.

          Gerrit-Project: chromium/src
          Gerrit-Branch: master
          Gerrit-MessageType: comment
          Gerrit-Change-Id: I56d11f001fc2717329b3caf0e815adac76b6ddb2
          Gerrit-Change-Number: 828734
          Gerrit-PatchSet: 3
          Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
          Gerrit-Reviewer: Commit Bot <commi...@chromium.org>
          Gerrit-Reviewer: Dirk Pranke <dpr...@chromium.org>
          Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
          Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
          Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
          Gerrit-CC: Mads Ager <ag...@chromium.org>
          Gerrit-Comment-Date: Mon, 18 Dec 2017 07:33:25 +0000
          Gerrit-HasComments: No
          Gerrit-HasLabels: No

          Michael Lippautz (Gerrit)

          unread,
          Dec 19, 2017, 11:07:21 AM12/19/17
          to blink-...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-...@chromium.org, Commit Bot, Jeremy Roman, Kentaro Hara, Dirk Pranke, chromium...@chromium.org, Findit, Mads Ager

          jbroman: fyi, this seems to work now :)

          View Change

            To view, visit change 828734. To unsubscribe, or for help writing mail filters, visit settings.

            Gerrit-Project: chromium/src
            Gerrit-Branch: master
            Gerrit-MessageType: comment
            Gerrit-Change-Id: I56d11f001fc2717329b3caf0e815adac76b6ddb2
            Gerrit-Change-Number: 828734
            Gerrit-PatchSet: 5
            Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
            Gerrit-Reviewer: Commit Bot <commi...@chromium.org>
            Gerrit-Reviewer: Dirk Pranke <dpr...@chromium.org>
            Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
            Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
            Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
            Gerrit-CC: Mads Ager <ag...@chromium.org>
            Gerrit-Comment-Date: Tue, 19 Dec 2017 16:07:16 +0000
            Gerrit-HasComments: No
            Gerrit-HasLabels: No

            Jeremy Roman (Gerrit)

            unread,
            Dec 19, 2017, 11:22:01 AM12/19/17
            to Michael Lippautz, blink-...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-...@chromium.org, Jeremy Roman, Commit Bot, Kentaro Hara, Dirk Pranke, chromium...@chromium.org, Findit, Mads Ager

            hooray \o/

            Patch set 5:Code-Review +1

            View Change

              To view, visit change 828734. To unsubscribe, or for help writing mail filters, visit settings.

              Gerrit-Project: chromium/src
              Gerrit-Branch: master
              Gerrit-MessageType: comment
              Gerrit-Change-Id: I56d11f001fc2717329b3caf0e815adac76b6ddb2
              Gerrit-Change-Number: 828734
              Gerrit-PatchSet: 5
              Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
              Gerrit-Reviewer: Commit Bot <commi...@chromium.org>
              Gerrit-Reviewer: Dirk Pranke <dpr...@chromium.org>
              Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
              Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
              Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
              Gerrit-CC: Mads Ager <ag...@chromium.org>
              Gerrit-Comment-Date: Tue, 19 Dec 2017 16:22:00 +0000
              Gerrit-HasComments: No
              Gerrit-HasLabels: Yes

              Michael Lippautz (Gerrit)

              unread,
              Dec 19, 2017, 12:58:56 PM12/19/17
              to blink-...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-...@chromium.org, Commit Bot, Jeremy Roman, Kentaro Hara, Dirk Pranke, chromium...@chromium.org, Findit, Mads Ager

              Patch set 5:Commit-Queue +2

              View Change

                To view, visit change 828734. To unsubscribe, or for help writing mail filters, visit settings.

                Gerrit-Project: chromium/src
                Gerrit-Branch: master
                Gerrit-MessageType: comment
                Gerrit-Change-Id: I56d11f001fc2717329b3caf0e815adac76b6ddb2
                Gerrit-Change-Number: 828734
                Gerrit-PatchSet: 5
                Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
                Gerrit-Reviewer: Commit Bot <commi...@chromium.org>
                Gerrit-Reviewer: Dirk Pranke <dpr...@chromium.org>
                Gerrit-Reviewer: Jeremy Roman <jbr...@chromium.org>
                Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
                Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
                Gerrit-CC: Mads Ager <ag...@chromium.org>
                Gerrit-Comment-Date: Tue, 19 Dec 2017 17:58:54 +0000
                Gerrit-HasComments: No
                Gerrit-HasLabels: Yes

                Commit Bot (Gerrit)

                unread,
                Dec 19, 2017, 1:05:52 PM12/19/17
                to Michael Lippautz, blink-...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-...@chromium.org, Jeremy Roman, Kentaro Hara, Dirk Pranke, chromium...@chromium.org, Findit, Mads Ager

                Commit Bot merged this change.

                View Change

                Approvals: Jeremy Roman: Looks good to me Kentaro Hara: Looks good to me Michael Lippautz: Commit
                Reland "[oilpan] Move incremental marking behind a build-time flag"

                This is a reland of 9966b73a986a5de99d3ba51d0d587b17fd0b938d
                Original change's description:
                > [oilpan] Move incremental marking behind a build-time flag
                >
                > Move Oilpan's incremental marking work behind a build-time flag
                > enable_blink_heap_incremental_marking
                >
                > Bug: chromium:757440
                > Change-Id: Id4adddcf0669dc165935eced90e8caefbb76a094
                > Reviewed-on: https://chromium-review.googlesource.com/822492
                > Commit-Queue: Michael Lippautz <mlip...@chromium.org>
                > Reviewed-by: Kentaro Hara <har...@chromium.org>
                > Reviewed-by: Dirk Pranke <dpr...@chromium.org>
                > Reviewed-by: Jeremy Roman <jbr...@chromium.org>
                > Cr-Commit-Position: refs/heads/master@{#524051}

                Bug: chromium:757440
                Change-Id: I56d11f001fc2717329b3caf0e815adac76b6ddb2
                Reviewed-on: https://chromium-review.googlesource.com/828734
                Reviewed-by: Jeremy Roman <jbr...@chromium.org>
                Reviewed-by: Kentaro Hara <har...@chromium.org>
                Commit-Queue: Michael Lippautz <mlip...@chromium.org>
                Cr-Commit-Position: refs/heads/master@{#525079}
                ---
                M third_party/WebKit/Source/platform/BUILD.gn
                M third_party/WebKit/Source/platform/heap/BUILD.gn
                M third_party/WebKit/Source/platform/heap/IncrementalMarkingTest.cpp
                M third_party/WebKit/Source/platform/heap/Member.h
                M third_party/WebKit/Source/platform/heap/ThreadState.cpp
                5 files changed, 25 insertions(+), 9 deletions(-)


                To view, visit change 828734. To unsubscribe, or for help writing mail filters, visit settings.

                Gerrit-Project: chromium/src
                Gerrit-Branch: master
                Gerrit-MessageType: merged
                Gerrit-Change-Id: I56d11f001fc2717329b3caf0e815adac76b6ddb2
                Gerrit-Change-Number: 828734
                Gerrit-PatchSet: 6
                Reply all
                Reply to author
                Forward
                0 new messages