Revert of Remove nonsfi token workaround.

Revert of Remove nonsfi token workaround. (patchset #1 id:20001 of https://codereview.chromium.org/807193006/)

Reason for revert:
This change caused huge ARC boot time regression on nonsfi mode (~600ms -> 900ms).
See: go/arcperf

Original issue's description:
> Remove nonsfi token workaround.
>
> Previously, file tokens were exposed to nonsfi plugins. Now that this is fixed
> (and we have test coverage for a nonsfi packaged app), we can remove the
> workaround that allowed nonzero token values.
>
> BUG=394130
> TEST=browser_tests on linux_rel_precise32
>
> Committed: https://crrev.com/4923bbcd251b3cf3519bc1251e26f8243db5ea02
> Cr-Commit-Position: refs/heads/master@{#309916}

TBR=mseaborn@chromium.org,teravest@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=394130
Committed: https://crrev.com/c68ee623b8306e00c4cb6f141545a9b2afd4bc67
Cr-Commit-Position: refs/heads/master@{#310250}

[Shuhei Takahashi, 2015-01-07 08:13:29]

Created Revert of Remove nonsfi token workaround.

[Shuhei Takahashi, 2015-01-07 08:15:31]

nya@chromium.org changed reviewers:
+ toyoshim@chromium.org

[Takashi Toyoshima, 2015-01-07 08:17:10]

LGTM.

Justin: Sorry for reverting this, but could you investigate what causes this
performance regression?

[Shuhei Takahashi, 2015-01-07 08:17:20]

The CQ bit was checked by nya@chromium.org

[commit-bot: I haz the power, 2015-01-07 08:18:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/836273002/1

[commit-bot: I haz the power, 2015-01-07 08:19:15]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-01-07 08:20:10]

Patchset 1 (id:??) landed as
https://crrev.com/c68ee623b8306e00c4cb6f141545a9b2afd4bc67
Cr-Commit-Position: refs/heads/master@{#310250}

[teravest, 2015-01-07 15:27:51]

How do I run the performance test that saw the performance change?
I tried going to go/arcperf, but the "Select a test suite" dropdown doesn't do
anything, and there's nothing listed in the main pane of the page.

[Mark Seaborn, 2015-01-07 16:15:05]

As a matter of process, I think you shouldn't be TBRing changes like this. 
TBRing is for greening the tree.  You could have afforded to wait for a review.

[Takashi Toyoshima, 2015-01-08 04:20:22]

Sorry for the immediate revert, but the regression was serious and blocked our
Chrome roll.
It increased ARC boot time from 525ms to 863ms (+65%).
I think reverting a change that causes performance or memory usage regression
isn't bad manner, but the problem in this case was the test suite wasn't a part
of chromium project. NaCl and ARC should collaborate closely, we may want to
have something to work effectively on this kind of problem.

> I tried going to go/arcperf, but the "Select a test suite" dropdown doesn't do
> anything, and there's nothing listed in the main pane of the page.

I guess you didn't login with Google account. As far as I checked now, login
page looks not working as expected. Here is the step how I login to the page
correctly.

1. login from the top page https://chromeperf.appspot.com/ with google account
2. open go/arcperf
3. you will see graphs for each architectures. the first graph for
bare_metal-i686-lkgr-perf had the performance drop, from 525ms to 863ms.

Do you have a environment to build ARC? If you do not, maybe asking to ARC
member in MTV will be quick because of the timezone difference. Sorry for a hard
process to reproduce it. Since we do not have good performance test suites in
chromium repository, this is the only way for now. Anyway, I'll send a mail for
the starting point separately.

[teravest, 2015-01-09 18:25:51]

The performance regression must mean that we're using the fast path
(in OpenNaClExecutable()) for ARC.

So, presumably, to remove the token workaround, the browser handler
for NaClHostMsg_OpenNaClExecutable should be modified for nonsfi mode
so that DoRegisterOpenedNaClExecutableFile()/PutFilePath() is never
called (and tokens are never generated).

On Wed, Jan 7, 2015 at 9:20 PM,  <toyoshim@chromium.org> wrote:
> Sorry for the immediate revert, but the regression was serious and blocked
> our
> Chrome roll.
> It increased ARC boot time from 525ms to 863ms (+65%).
> I think reverting a change that causes performance or memory usage
> regression
> isn't bad manner, but the problem in this case was the test suite wasn't a
> part
> of chromium project. NaCl and ARC should collaborate closely, we may want to
> have something to work effectively on this kind of problem.
>
>> I tried going to go/arcperf, but the "Select a test suite" dropdown
>> doesn't do
>> anything, and there's nothing listed in the main pane of the page.
>
>
> I guess you didn't login with Google account. As far as I checked now, login
> page looks not working as expected. Here is the step how I login to the page
> correctly.
>
> 1. login from the top page https://chromeperf.appspot.com/ with google
> account
> 2. open go/arcperf
> 3. you will see graphs for each architectures. the first graph for
> bare_metal-i686-lkgr-perf had the performance drop, from 525ms to 863ms.
>
> Do you have a environment to build ARC? If you do not, maybe asking to ARC
> member in MTV will be quick because of the timezone difference. Sorry for a
> hard
> process to reproduce it. Since we do not have good performance test suites
> in
> chromium repository, this is the only way for now. Anyway, I'll send a mail
> for
> the starting point separately.
>
> https://codereview.chromium.org/836273002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.