[tracing] Set whitelist values for dump provider supporting background mode

[tracing] Set whitelist values for dump provider supporting background mode

Adds list of strings to whitelist.
Modifies stripping logic to make the whitelist readable.
The whitelisted dump providers support background mode efficiently.

BUG=613198
Committed: https://crrev.com/3f4573d82141b7e36438f84ab12fcf9a20725e1e
Cr-Commit-Position: refs/heads/master@{#400064}

[ssid, 2016-06-06 20:52:38]

Description was changed from

==========
Set whitelist values dump provider supporting background mode

BUG=
==========

to

==========
Set whitelist values for dump provider supporting background mode

BUG=613198
==========

[ssid, 2016-06-06 21:01:16]

Patchset #1 (id:1) has been deleted

[ssid, 2016-06-06 21:01:23]

Patchset #1 (id:20001) has been deleted

[ssid, 2016-06-06 21:07:07]

Patchset #1 (id:40001) has been deleted

[ssid, 2016-06-06 21:08:29]

Description was changed from

==========
Set whitelist values for dump provider supporting background mode

BUG=613198
==========

to

==========
[tracing] Set whitelist values for dump provider supporting background mode

BUG=613198
==========

[ssid, 2016-06-09 01:31:07]

Patchset #2 (id:80001) has been deleted

[ssid, 2016-06-09 01:43:34]

Description was changed from

==========
[tracing] Set whitelist values for dump provider supporting background mode

BUG=613198
==========

to

==========
[tracing] Set whitelist values for dump provider supporting background mode

Adds list of strings to whitelist.
Modifies stripping to make the whitelist readable.
The whitelisted dump providers support background mode efficiently.

BUG=613198
==========

[ssid, 2016-06-09 01:43:54]

Description was changed from

==========
[tracing] Set whitelist values for dump provider supporting background mode

Adds list of strings to whitelist.
Modifies stripping to make the whitelist readable.
The whitelisted dump providers support background mode efficiently.

BUG=613198
==========

to

==========
[tracing] Set whitelist values for dump provider supporting background mode

Adds list of strings to whitelist.
Modifies stripping logic to make the whitelist readable.
The whitelisted dump providers support background mode efficiently.

BUG=613198
==========

[ssid, 2016-06-09 01:46:59]

ssid@chromium.org changed reviewers:
+ primiano@chromium.org

[ssid, 2016-06-09 01:47:00]

+primiano
I finally managed to fix all the listed dump providers for background mode.
This slightly modifies the stripping logic as you suggested. I experimented with
strings and found this one looks the best. ptal, thanks.

The browser test for this CL is here:
https://codereview.chromium.org/2047533002/.

Note that this CL will wait till that cl is green.

[Primiano Tucci (use gerrit), 2016-06-09 18:12:00]

the whilelist looks sane to me.
Just some final comment.... sorry for being  a pain on this but this I'm really
paranoid on this thing :)

https://codereview.chromium.org/2012883003/diff/100001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2012883003/diff/100001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:102: if (parsing_hex) {
Sorry for the back an forths, trying to make this more easy to read and
maintain.
1) Do you really need to strip digits (on top of 0xpointers?) It doesn't seem to
be the case here. I'll defer that until we need it.
2) Can we make this more explicit and easy to read as follows:

for (i...)
   if (parsing_hex && isxdigit())
     continue
   parsing_hex = false;
    if (i + 1 < length && name[i] == '0' && name[i + 1] == 'x') {
      parsing_hex = true;
       push_back("0x?")
    } else {
     push_back(name[i])
     }

In this way:
- v8 will stay as v8
- the whitelist of the isolate will be less obscure, as it will look like
v8/isolate_0x?/...
- you don't have the double // problem on leveldb

WDYT?

[ssid, 2016-06-09 22:23:13]

https://codereview.chromium.org/2012883003/diff/100001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2012883003/diff/100001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:102: if (parsing_hex) {
On 2016/06/09 18:12:00, Primiano Tucci wrote:
> the whilelist looks sane to me.
> Just some final comment.... sorry for being  a pain on this but this I'm
really
> paranoid on this thing :)
No problem I was expecting suggestion on this.

>
https://codereview.chromium.org/2012883003/diff/100001/base/trace_event/memor...
> File base/trace_event/memory_infra_background_whitelist.cc (right):
> 
>
https://codereview.chromium.org/2012883003/diff/100001/base/trace_event/memor...
> base/trace_event/memory_infra_background_whitelist.cc:102: if (parsing_hex) {
> Sorry for the back an forths, trying to make this more easy to read and
> maintain.
> 1) Do you really need to strip digits (on top of 0xpointers?) It doesn't seem
to
> be the case here. I'll defer that until we need it.

Yeah I had to strip digits before I fixed dump providers for background mode.
Discardable was adding numbers.

> 2) Can we make this more explicit and easy to read as follows:
> 
> for (i...)
>    if (parsing_hex && isxdigit())
>      continue
>    parsing_hex = false;
>     if (i + 1 < length && name[i] == '0' && name[i + 1] == 'x') {
>       parsing_hex = true;
>        push_back("0x?")
>     } else {
>      push_back(name[i])
>      }
> 
> In this way:
> - v8 will stay as v8
> - the whitelist of the isolate will be less obscure, as it will look like
> v8/isolate_0x?/...
> - you don't have the double // problem on leveldb
> 
> WDYT?

Yeah makes sense, I do not have to worry about double puncts in the strings.
thanks, changed.

[Primiano Tucci (use gerrit), 2016-06-10 08:24:45]

Excellent, thanks a lot, LGTM.

Out of curiosity, IIRC you did also some work on the discardable dumper, but I
don't see it here.

[ssid, 2016-06-11 00:50:33]

On 2016/06/10 08:24:45, Primiano Tucci wrote:
> Excellent, thanks a lot, LGTM.
> 
> Out of curiosity, IIRC you did also some work on the discardable dumper, but I
> don't see it here.

Urg, So windows does not add "0x" for %p formats!
We need to figure out some other way to do this :( I currently can't think of
any way to strip the hex digits without '0x', except that windows writes the hex
number using upper case alphabets and always 16 characters.
Keeping this patch on hold.

[Primiano Tucci (use gerrit), 2016-06-13 19:52:29]

On 2016/06/11 00:50:33, ssid wrote:
> On 2016/06/10 08:24:45, Primiano Tucci wrote:
> > Excellent, thanks a lot, LGTM.
> > 
> > Out of curiosity, IIRC you did also some work on the discardable dumper, but
I
> > don't see it here.
> 
> Urg, So windows does not add "0x" for %p formats!
> We need to figure out some other way to do this :( I currently can't think of
> any way to strip the hex digits without '0x', except that windows writes the
hex
> number using upper case alphabets and always 16 characters.
> Keeping this patch on hold.

Ouch. Can we change those providers to be "0x" PRIXPTR instead of "%p".
Regardless of background tracing would be nice to have them being consistent
cross-os. Didn't know that %p was inconsistent.
Should be easy to fix across the board and a TBR-able

[ssid, 2016-06-13 20:02:28]

On 2016/06/13 19:52:29, Primiano Tucci wrote:
> On 2016/06/11 00:50:33, ssid wrote:
> > On 2016/06/10 08:24:45, Primiano Tucci wrote:
> > > Excellent, thanks a lot, LGTM.
> > > 
> > > Out of curiosity, IIRC you did also some work on the discardable dumper,
but
> I
> > > don't see it here.
> > 
> > Urg, So windows does not add "0x" for %p formats!
> > We need to figure out some other way to do this :( I currently can't think
of
> > any way to strip the hex digits without '0x', except that windows writes the
> hex
> > number using upper case alphabets and always 16 characters.
> > Keeping this patch on hold.
> 
> Ouch. Can we change those providers to be "0x" PRIXPTR instead of "%p".
> Regardless of background tracing would be nice to have them being consistent
> cross-os. Didn't know that %p was inconsistent.
> Should be easy to fix across the board and a TBR-able

Oh yeah I was thinking of detecting hex numbers using consecutive pointer sized
number. But changing the dump providers sounds like better option.

[ssid, 2016-06-13 20:45:35]

Patchset #6 (id:180001) has been deleted

[ssid, 2016-06-15 22:31:59]

Since the cq is tests are green in https://codereview.chromium.org/2047533002/ I
ticking this one, as well as the benchmark at
https://codereview.chromium.org/2052753002/.
Thanks

[ssid, 2016-06-15 22:39:57]

The CQ bit was checked by ssid@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 22:40:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2012883003/200001

[commit-bot: I haz the power, 2016-06-16 00:15:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-16 00:15:16]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-06-16 00:56:05]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-06-16 00:56:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2012883003/#ps200001
(title: "Rebase on 2067543003.")

[commit-bot: I haz the power, 2016-06-16 00:56:50]

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

[commit-bot: I haz the power, 2016-06-16 01:01:00]

Description was changed from

==========
[tracing] Set whitelist values for dump provider supporting background mode

Adds list of strings to whitelist.
Modifies stripping logic to make the whitelist readable.
The whitelisted dump providers support background mode efficiently.

BUG=613198
==========

to

==========
[tracing] Set whitelist values for dump provider supporting background mode

Adds list of strings to whitelist.
Modifies stripping logic to make the whitelist readable.
The whitelisted dump providers support background mode efficiently.

BUG=613198
==========

[commit-bot: I haz the power, 2016-06-16 01:01:01]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-06-16 01:01:05]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 01:02:53]

Description was changed from

==========
[tracing] Set whitelist values for dump provider supporting background mode

Adds list of strings to whitelist.
Modifies stripping logic to make the whitelist readable.
The whitelisted dump providers support background mode efficiently.

BUG=613198
==========

to

==========
[tracing] Set whitelist values for dump provider supporting background mode

Adds list of strings to whitelist.
Modifies stripping logic to make the whitelist readable.
The whitelisted dump providers support background mode efficiently.

BUG=613198

Committed: https://crrev.com/3f4573d82141b7e36438f84ab12fcf9a20725e1e
Cr-Commit-Position: refs/heads/master@{#400064}
==========

[commit-bot: I haz the power, 2016-06-16 01:02:55]

Patchset 6 (id:??) landed as
https://crrev.com/3f4573d82141b7e36438f84ab12fcf9a20725e1e
Cr-Commit-Position: refs/heads/master@{#400064}

[ssid, 2016-09-07 03:43:47]

Patchset #1 (id:60001) has been deleted

[ssid, 2016-09-07 03:43:55]

Patchset #4 (id:160001) has been deleted

[ssid, 2016-09-07 03:44:05]

Patchset #2 (id:120001) has been deleted