Bug 103688 – IndexedDB: Make leveldb histogram entries more fine-grained

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: Each place of failure should have its own histogram entry

[David Grogan]

Created attachment 176853 [details]
Patch

[David Grogan]

Josh and/or Alec, could you take a look at this?

Given how little we know about what's happening, I want to go with this brute force approach instead of trying to be too intelligent. Counterproposals welcome of course.

[David Grogan]

Michael did something similar with WebSQL callsites.  His was more structured though, he broke them out by operation type, and then by callsite.

http://go/michaelswebsqlstats

[David Grogan]

Created attachment 177007 [details]
Patch

updated to ToT

[David Grogan]

Tony, could you review this?

After some uncertainty we (jsbell+alecflett+me) agreed in person that this approach was fine. Your thoughts are also welcome.

[Tony Chang]

Each value you add takes additional memory, so trying to keep the number of values small will help a bit.

I'm not sure if you plan on reusing/renumbering values or just keep adding.  It seems like it would be confusing as the code changes to keep track of what values are needed.

So while this works, I think it would be a bit better if instead of just numbering the errors, you named them and perhaps grouped them.  For example, you could have a distinct error for each function and name the enum values to match the function names.  This seems easier to maintain going forward.

On the other hand, maybe you plan on removing this after one or two dev channel release, in which case, maybe it doesn't matter.

[David Grogan]

Created attachment 177340 [details]
Patch

[David Grogan]

This patch switches to the one-enum-entry-per-function approach. Tony, is this what you had in mind?

[Tony Chang]

Comment on attachment 177340 [details]
Patch

Yes, that's what I had in mind.

[Alec Flett]

just coming late to this, (maybe too late) but how do the additional values use more memory? I think we can safely assume that these enum values are just going to get hardcoded into the compiled instructions, so there shouldn't be any difference between 

movl eax, 0x12

and 

movl eax, 0x23

[WebKit Review Bot]

Comment on attachment 177340 [details]
Patch

Rejecting attachment 177340 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ripts/update-webkit line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
   9961a7b..ef0ca77  HEAD       -> origin/HEAD
error: Ref refs/remotes/origin/master is at ef0ca77a554ed2a07b001ee4935266d6301b2bf8 but expected 9961a7b754078ea893b086fbc787c57745c8a4db
 ! 9961a7b..ef0ca77  master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15117748

[WebKit Review Bot]

Comment on attachment 177340 [details]
Patch

Rejecting attachment 177340 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ripts/update-webkit line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
   54297d7..837f7c4  HEAD       -> origin/HEAD
error: Ref refs/remotes/origin/master is at 837f7c4da6fc1cce27d8517a8319c0e408da63da but expected 54297d74214132a39292f05bb9466fdab588b0aa
 ! 54297d7..837f7c4  master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15121656

[WebKit Review Bot]

Comment on attachment 177340 [details]
Patch

Clearing flags on attachment: 177340

Committed r136497: <http://trac.webkit.org/changeset/136497>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Grogan]

(In reply to comment #10)
> just coming late to this, (maybe too late) but how do the additional values use more memory? I think we can safely assume that these enum values are just going to get hardcoded into the compiled instructions, so there shouldn't be any difference between 
> 
> movl eax, 0x12
> 
> and 
> 
> movl eax, 0x23

I'm assuming the histogram code allocates an int[] of size IDBLevelDBBackingStoreInternalErrorMax and then increments each entry as we report it, so the more error codes we have the larger that array has to be.

But your argument might assume this and be more advanced, not sure. Is it?

[Tony Chang]

(In reply to comment #15)
> I'm assuming the histogram code allocates an int[] of size IDBLevelDBBackingStoreInternalErrorMax and then increments each entry as we report it, so the more error codes we have the larger that array has to be.

Yes, that's the extra memory I'm referring to.  I think we also keep a copy of the histogram in the browser process and each renderer process that adds to this histogram.