Open Bug 1588710 Opened 4 months ago Updated 2 months ago

Evaluate the performance impact of the Stack clash protection

Categories

(Firefox Build System :: General, task, P2)

Tracking

(Not tracked)

People

(Reporter: Sylvestre, Assigned: Sylvestre)

Details

(Keywords: sec-want)

Attachments

(1 file)

This is a feature provided by gcc for a few years now.
clang doesn't have it and it has been marked as a blocker for redhat/fedora to use clang to build Firefox.

Serge started the work on the clang side to implement it:
http://lists.llvm.org/pipermail/llvm-dev/2019-October/135786.html
https://reviews.llvm.org/D68720

He reached out to me to evaluate the change for Firefox performances.

Opening a bug for transparency and sharing results

If there isn't performance regression, we might want to add this to our official builds.

Is it possible for him to make a backport of the patch to the clang 8 branch? That would be easiest for us to apply and test.

With the perf results:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=3850d59d0d33e2ca0c746c54f059304306368972&framework=1&selectedTimeRange=172800

I do see an improvement from 3 to 4% on Linux.
I am a bit confused to see Windows improvement as I didn't touch it (the stack clash patch is only applied on clang Linux) but the confidence is smaller.
Anyway, it matches what Serge told me. He saw improvements on some projects (and no regression yet).
I would however appreciate a confirmation by an actual perf expert.

So, if this is the case, once landed, we could even take the patch to clang 9 to avoid having to wait a few more months (clang 10 release)

:jmaher is who I usually go to for such requests, but he is on leave.

Would Serge be open to improving his patch to include Windows? Or at least documenting what would be needed to do so?

Keywords: sec-want

Comparing try with try isn't giving the same kind of results (slower)
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&newProject=try&newRevision=3850d59d0d33e2ca0c746c54f059304306368972&framework=1&selectedTimeRange=172800

I am a bit lost on how to understand these results.

On dmajor suggestion, I reused 1db6f0e4ceca73e401ca60d0e93ad29cbe1c3949 as base line (the clang patch but not enabled in our builds) and retriggered the talos jobs

Here is the comparison:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1db6f0e4ceca73e401ca60d0e93ad29cbe1c3949&newProject=try&newRevision=3850d59d0d33e2ca0c746c54f059304306368972&framework=1

Seems that there isn't any significant improvement or regression after all. Except maybe a regression in "about_preferences_basic opt e10s stylo" on Linux opt (not pgo)

Please also run raptor tests, at least speedometer. Should we also test this on Windows builds?

Serge told me that it should not have impact on Windows (and not needed there)

@dmajor, @tjr : The windows ABI already requires a touch on each stack page, so Windows shouldn't be affected, (not the case of the patch you're testing with, but should be the case in the near future). Basically it means that clang-cl will support -fstack-clash-protection, but it should be a no-op for Windows.

@sylvestre: thanks for the feedback. Having firefox passing its validation with this patch is very good news. I've made several significant changes to the original patch, so an extra round may be needed, keep in touch.

@all: once the patch lands in llvm's master, I'll provide a backport if you want.

Seems that there isn't any performance change (but I might be wrong as I am no expert in perfherder)

Assignee: nobody → sledru

Dave Hunt told me the following:

If so, this suggests that your patch has caused a high confidence regression to the kraken results across all platforms. The kraken benchmark measures JavaScript performance, details can be found at https://wiki.mozilla.org/TestEngineering/Performance/Talos/Tests#kraken. There also appears to be a high confidence improvement to tabpaint across all platforms, which is documented here: https://wiki.mozilla.org/TestEngineering/Performance/Talos/Tests#tabpaint. There are a few other medium to high confidence improvements and regressions.

I see references to linux64, macos64, and win64 in your patch here https://hg.mozilla.org/try/rev/408e7270af623961e8cb466aa31b18248aa3e512 could these explain the cross-platform impact? Were the tasks executed around the same time? The reason I ask is that if the base tasks were triggered first, and the new triggered much later, perhaps an infrastructure change could have been introduced in the meantime.

Serge told me that he will have a look why Windows performances are impacted

After discussing with dmajor, he told me that I have been probably impacted by another unrelated issue.

So, comparing with and without the option activated, I saw that:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=11d4b305e91184937dc2a2f3db7974653faabf87&newProject=try&newRevision=90d5778298672bc8e1b7f961606225ca9a867330&framework=1

Seems that we are only regressing on Windows 7:
tp5n main_startup_fileio opt e10s stylo
tp5n time_to_session_store_window_restored_ms opt e10s stylo

Attached image image.png

for legacy

Seems that we are only regressing on Windows 7:
tp5n main_startup_fileio opt e10s stylo
tp5n time_to_session_store_window_restored_ms opt e10s stylo

I'm not worried about these two, historically they haven't been super reliable for me.

@sylvestre: I've explicitly disabled the flag for non-Linux targets, users receive a warning if they try to use it for non-Linux, non-x86:x64 platform. I've also setup an extra validation step for the review, it should be fine : https://github.com/serge-sans-paille/llvm-project/pull/3/checks

Serge provided me with a new patch to skip the execution of this code on Windows & Mac.

I just started some talos job;

  1. without the option enabled:
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=b67d19f2d016cfd19e2854fdd2334c7540a96b31
    (just clang + a normal fx build)
    https://hg.mozilla.org/try/rev/41b41afc9bc18557a86db05becdb0f6b6f154a10

  2. with the option activated:
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=a405edf572eb19ba33b829bb691a3a3095e68cc6
    https://hg.mozilla.org/try/rev/cc533026241361425a0991c01e4bff3bb4239605

I kept the Windows & Mac part to make sure that it behaves correctly

Looking at the results ( https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a405edf572eb19ba33b829bb691a3a3095e68cc6&newProject=try&newRevision=b67d19f2d016cfd19e2854fdd2334c7540a96b31&framework=1 ) . I don't seen any regression on Windows or Mac anymore

Just a small regression with displaylist_mutate opt e10s stylo on Linux 64 (-1.2%)

(In reply to Sylvestre Ledru [:Sylvestre] from comment #20)

Looking at the results ( https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a405edf572eb19ba33b829bb691a3a3095e68cc6&newProject=try&newRevision=b67d19f2d016cfd19e2854fdd2334c7540a96b31&framework=1 ) . I don't seen any regression on Windows or Mac anymore

Just a small regression with displaylist_mutate opt e10s stylo on Linux 64 (-1.2%)

It looks like there are only five runs before and after. At that level, we can't draw a conclusion because the tests have too much variance. Also there aren't any raptor runs in there. Depending on how intense you want to be about this experiment, I would say we need more data. But if I understand correctly this is sort of just a casual run, so maybe this combined with the previous testing is sufficient.

You need to log in before you can comment on or make changes to this bug.