Log IDB leveldb uma stats into LevelDBEnv.IDB. Other LevelDB consumers will

Log IDB leveldb uma stats into LevelDBEnv.IDB.

Other LevelDB consumers will still be aggregated under LevelDBEnv.

BUG=164755
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175432

[dgrogan, 2013-01-04 23:05:35]

Alec and/or Josh, could you review this?

I was unable, after a few feeble attempts, to make leveldb enter a tight loop
trying to open an sst file that's not there. This CL will tell us if that loop
is happening in one of the other leveldb consumers.

https://bugs.webkit.org/show_bug.cgi?id=106135 will need to  land to use the new
environment.

[jsbell, 2013-01-05 00:14:54]

lgtm with some nits and a bit of paranoia...

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:129: UMALogger(std::string uma_title)
: uma_title_(uma_title) {
Nit: put the closing brace on the same line.

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:206: const UMALogger* uma_logger_;
How certain are we that these never outlive the Env?

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:209: ChromiumSequentialFile(const
std::string& fname, FILE* f, const UMALogger* uma_logger)
Nit: Line length.

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:245: ChromiumRandomAccessFile(const
std::string& fname, ::base::PlatformFile file, const UMALogger* uma_logger)
Nit: line length.

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:270: ChromiumWritableFile(const
std::string& fname, FILE* f, const UMALogger* uma_logger)
Nit: line length.

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:649: uma_logger_.reset(new
UMALogger("LevelDBEnv.IDB"));
Maybe make it a constructor parameter in ChromiumEnv with a default value to
avoid the churn? (Not important in the grand scheme of things...)

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:661: Env* IDBEnv() {
FYI, this may cause some valgrind leak suppressions to stop matching. Will
involve some follow-up work to update. http://crbug.com/140860 has the links I
think.

[jsbell, 2013-01-05 00:19:05]

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:649: uma_logger_.reset(new
UMALogger("LevelDBEnv.IDB"));
Actually, why not have ChromiumEnv take a const char* argument e.g.
logger_prefix, with a default value of "LevelDBEnv"? That would avoid having to
create IDBEnv; IDB would just supply a value over in the WebKit side, and other
Chromium leveldb clients could supply their own as well.

[dgrogan, 2013-01-05 01:05:27]

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:206: const UMALogger* uma_logger_;
On 2013/01/05 00:14:54, jsbell wrote:
> How certain are we that these never outlive the Env?

Env is never deleted (its destructor is NOTREACHED) so this should be ok.

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:209: ChromiumSequentialFile(const
std::string& fname, FILE* f, const UMALogger* uma_logger)
On 2013/01/05 00:14:54, jsbell wrote:
> Nit: Line length.

Oops. Normally the presubmit catches this. Maybe it skips stuff in third_party.

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:649: uma_logger_.reset(new
UMALogger("LevelDBEnv.IDB"));
On 2013/01/05 00:19:05, jsbell wrote:
> Actually, why not have ChromiumEnv take a const char* argument e.g.
> logger_prefix, with a default value of "LevelDBEnv"? That would avoid having
to
> create IDBEnv; IDB would just supply a value over in the WebKit side, and
other
> Chromium leveldb clients could supply their own as well.

I set out to do something like what you're describing. I went this way because
of LazyInstance. It looked like passing a parameter to the constructor would be
ugly, if possible at all.

An alternative would be to have a map of envs, but it seemed like overkill.
Though getting rid of the extra IDBEnv class would be nice. Thoughts?

LazyInstance<map<string name, ChromiumEnv> > envs;
Lock envs_lock;

Env* getNamedEnv(name) {
  AutoLock lock(envs_lock);
  if (!envs[name])
     envs[name] = new ChromiumEnv(name);
  return envs[name];
}

Env* Env::Default() {
  return getNamedEnv("default");
}

[dgrogan, 2013-01-07 18:49:12]

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:129: UMALogger(std::string uma_title)
: uma_title_(uma_title) {
On 2013/01/05 00:14:54, jsbell wrote:
> Nit: put the closing brace on the same line.

Done.

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:245: ChromiumRandomAccessFile(const
std::string& fname, ::base::PlatformFile file, const UMALogger* uma_logger)
On 2013/01/05 00:14:54, jsbell wrote:
> Nit: line length.

Done.

https://codereview.chromium.org/11776009/diff/2001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:270: ChromiumWritableFile(const
std::string& fname, FILE* f, const UMALogger* uma_logger)
On 2013/01/05 00:14:54, jsbell wrote:
> Nit: line length.

Done.

[commit-bot: I haz the power, 2013-01-07 18:50:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/11776009/5

[commit-bot: I haz the power, 2013-01-07 21:20:52]

Retried try job too often on win_aura for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-07 22:33:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/11776009/5

[commit-bot: I haz the power, 2013-01-08 00:38:23]

Change committed as 175432