Unrevert r245135 "Created new Windows LevelDB environment."

The original implementation was syncing file contents to disk
when only a flush was required.

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

[cmumford, 2013-12-11 22:46:06]

dgrogan@chromium.org: Please review changes in 

alecflett@chromium.org: Please review changes in 

jsbell@chromium.org: Please review changes in 

Finally!

[jsbell, 2013-12-12 01:11:42]

Just some initial notes.

The side-by-side diff isn't working; did the upload choke?

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_posix.cc (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_posix.cc:261:
ChromiumEnvPosix::ChromiumEnvPosix() {
Naming: Calling it Posix is confusing, since it's not POSIX specific - it's
using the PlatformFile abstractions. Eventually it may be POSIX-specific, but we
can rename then.

Not that I have great alternatives. ChromiumEnvPlatform ?

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_win32.cc (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:35:
|FORMAT_MESSAGE_ALLOCATE_BUFFER
nit: spaces around the | operator, and wrap the line after the operator. Try
using "git cl format" to apply clang-format to the patch.

FYI: http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:109: ::base::PlatformFile file_;
Isn't the point of this patch to use Windows APIs directly, not the
base::PlatformFile abstraction?

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:136: const std::string& message,
indentation

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:141: sizeof(buf),
indentation

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:149: else {
else should be on the same line as the closing brace, i.e. } else {

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:155: HANDLE f,
indentation (etc)

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:186: return Status();
Add a comment here that SyncParent() is (apparently) not a concept that applies
on Win32.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_win32.h (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.h:17: class
ChromiumWritableFileWin32 : public leveldb::WritableFile {
Naming: We normally just use "win" not "win32". (I think we say "win32" in
conversation to make it clear what we're talking about, but not in
Chromium/Blink identifiers.)

[cmumford, 2013-12-12 17:40:50]

Address Josh's comments.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_posix.cc (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_posix.cc:261:
ChromiumEnvPosix::ChromiumEnvPosix() {
On 2013/12/12 01:11:42, jsbell wrote:
> Naming: Calling it Posix is confusing, since it's not POSIX specific - it's
> using the PlatformFile abstractions. Eventually it may be POSIX-specific, but
we
> can rename then.
> 
> Not that I have great alternatives. ChromiumEnvPlatform ?

So ChromiumEnv calls the file abstractions in base. ChromiumEnvWin32 is direct
Win32, and ChromiumEnvPosix does FILE stream I/O (AKA stdio) which is part of
POSIX. Also, an instance of ChromiumEnvPosix should now do *only* POSIX I/O, and
ChromiumEnvWin32 will do only Win32 (with one exception). Did I get that right?

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_win32.cc (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:35:
|FORMAT_MESSAGE_ALLOCATE_BUFFER
On 2013/12/12 01:11:42, jsbell wrote:
> nit: spaces around the | operator, and wrap the line after the operator. Try
> using "git cl format" to apply clang-format to the patch.
> 
> FYI: http://www.chromium.org/developers/coding-style

thx. didn't know abot git cl format. Next patch will be "git cl" formatted.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:109: ::base::PlatformFile file_;
On 2013/12/12 01:11:42, jsbell wrote:
> Isn't the point of this patch to use Windows APIs directly, not the
> base::PlatformFile abstraction?

I can do that if you prefer. This is the first sentence from the bug "This could
involve rewriting all of ChromiumEnv to use ::base or just surrounding some
windows CreateFile-style code in #ifdefs." So I think this meets that criteria,
but if you want the Win32 env to be direct to the Win32 API without using base::
let me know.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:186: return Status();
On 2013/12/12 01:11:42, jsbell wrote:
> Add a comment here that SyncParent() is (apparently) not a concept that
applies
> on Win32.

Will do. I found the original review
(https://chromiumcodereview.appspot.com/11445030) and have added his rationale
as a comment in this function.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_win32.h (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.h:17: class
ChromiumWritableFileWin32 : public leveldb::WritableFile {
On 2013/12/12 01:11:42, jsbell wrote:
> Naming: We normally just use "win" not "win32". (I think we say "win32" in
> conversation to make it clear what we're talking about, but not in
> Chromium/Blink identifiers.)

Good point. *Win would be more consistent with the rest of the code. I'll change
that.

[jsbell, 2013-12-12 18:11:40]

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_posix.cc (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_posix.cc:261:
ChromiumEnvPosix::ChromiumEnvPosix() {
It's more about how the terms "win" and "posix" are used in Chromium. Generally,
"win" = windows specific, and "posix" = unix-like operating systems, i.e. linux,
mac osx, etc; practically speaking posix == !win. So it would be surprising for
a Chromium developer unfamiliar with this code to find that ChromiumEnvPosix
could be instantiated/used on Windows.

[jsbell, 2013-12-12 18:12:48]

On 2013/12/12 01:11:42, jsbell wrote:
> The side-by-side diff isn't working; did the upload choke?

My bad - it was just the size of the grd file making rietveld sad. If I start
the review on a later file I get lovely side-by-side diffs.

[jsbell, 2013-12-12 19:03:13]

Sorry for the very scattered feedback. It's taking a while for things to soak
in.

I suggest splitting the patch to remove the command line flags for now, so that
the change is just the env implementation and test support, even if there's no
way to turn it on. We'll probably want other reviewers the flags/finch
settings/etc anyway.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium.h (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium.h:100: class ChromiumEnvDelegate
It's unfortunate to introduce this just for unit tests. Can this be done by
subclassing/overriding instead? i.e. have the test derive TestChromiumEnvFoo
from ChromiumEnvFoo, and override DidSyncDir() and expose directory_syncs() ?

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_win32.cc (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:40: if (errorText[0] != L'\0') {
Is there a reason to expect trailing whitespace here? (i.e. does a newline
typically get appended or something?) If so, can you add a comment?

(I'd feel more comfortable with the "trim" operation occurring on the
std::string after the conversion myself - maybe implement a separate static
Trim(std::string*) helper?)

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:56: class
ChromiumSequentialFile: public SequentialFile {
I'm slightly nervous that the re-use of this name (ChromiumSequentialFile)
between the win32 and other env will lead to confusion. They're scoped by the
anonymous namespace in their respective compilation units so it's not a
technical issue, but may confuse someone grepping/debugging the code.

I don't feel strongly about it though.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:109: ::base::PlatformFile file_;
On 2013/12/12 17:40:51, cmumford wrote:
> I can do that if you prefer. This is the first sentence from the bug "This
could
> involve rewriting all of ChromiumEnv to use ::base or just surrounding some
> windows CreateFile-style code in #ifdefs." So I think this meets that
criteria,
> but if you want the Win32 env to be direct to the Win32 API without using
base::
> let me know.

I think we should go farther; one of our hypotheses for the high corruption rate
on Windows is that the base:: platform file abstractions are hiding something
that ends up being a bug due to implicit assumptions about the API contract. So
dropping down to raw Win32, and potentially comparing against other Win32-envs
for leveldb, is a goal of this effort. 

That wasn't captured in the bug and/or we've hijacked the meaning of the bug. :(

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:186: return Status();
On 2013/12/12 17:40:51, cmumford wrote:
> On 2013/12/12 01:11:42, jsbell wrote:
> > Add a comment here that SyncParent() is (apparently) not a concept that
> applies
> > on Win32.
> 
> Will do. I found the original review
> (https://chromiumcodereview.appspot.com/11445030) and have added his rationale
> as a comment in this function.

Thanks! dgrogan@ - should we just close crrev.com/19678017 ?

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:274: #if defined(OS_POSIX)
How could this ever be true?

We may want to leave a comment in this no-op method that Windows doesn't have
file limits in the same way as "POSIX"

[cmumford, 2013-12-12 19:37:46]

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_posix.cc (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_posix.cc:261:
ChromiumEnvPosix::ChromiumEnvPosix() {
On 2013/12/12 18:11:41, jsbell wrote:
> It's more about how the terms "win" and "posix" are used in Chromium.
Generally,
> "win" = windows specific, and "posix" = unix-like operating systems, i.e.
linux,
> mac osx, etc; practically speaking posix == !win. So it would be surprising
for
> a Chromium developer unfamiliar with this code to find that ChromiumEnvPosix
> could be instantiated/used on Windows.

TL;DR How about ChromiumEnvPosix -> ChromiumEnvStdio

Yes, I totally see your point, and agree that the assumption would be
ChromiumEnvPosix would not be instantiated on Windows. Unfortunately we have two
leaf classes:

1. PlatformFile(Win32/POSIX/etc.) + ChromiumEnv(POSIX)
2. PlatformFile(Win32) + ChromiumEnv(Win32)

And our class hierarchy is currently

ChromiumEnv
  ChromiumEnvPosix (#1 above)
  ChromiumEnvWin (#2 above)

We could do something like:

ChromiumEnv
  ChromiumEnvBase (#1 above)
  ChromiumEnvWin (#2 above)

But I think that most developers would expect a class with "Base" in it would be
a base class. Also, ChromiumEnv does "Base" (AKA PlatformFile) I/O, so that
would be doubly confusing.

We might also do:

ChromiumEnvBase
  ChromiumEnv (#1 above)
  ChromiumEnvWin (#2 above)

That would be a bit cleaner, but we would not have the normal symmetry
developers expect when looking at platform specific classes.

We could also do:

ChromiumEnv
  ChromiumEnvStdio (#1 above)
  ChromiumEnvWin (#2 above)

I'm warming to this last idea - Skia does this with SkOSFile_stdio.cpp

[dgrogan, 2013-12-12 19:39:56]

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_posix.cc (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_posix.cc:261:
ChromiumEnvPosix::ChromiumEnvPosix() {
On 2013/12/12 19:37:46, cmumford wrote:
> TL;DR How about ChromiumEnvPosix -> ChromiumEnvStdio

This SGTM.

[jsbell, 2013-12-12 19:40:42]

On 2013/12/12 19:39:56, dgrogan wrote:
>
https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
> File third_party/leveldatabase/env_chromium_posix.cc (right):
> 
>
https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
> third_party/leveldatabase/env_chromium_posix.cc:261:
> ChromiumEnvPosix::ChromiumEnvPosix() {
> On 2013/12/12 19:37:46, cmumford wrote:
> > TL;DR How about ChromiumEnvPosix -> ChromiumEnvStdio
> 
> This SGTM.

+1

[cmumford, 2013-12-12 20:08:38]

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium.h (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium.h:100: class ChromiumEnvDelegate
On 2013/12/12 19:03:14, jsbell wrote:
> It's unfortunate to introduce this just for unit tests. Can this be done by
> subclassing/overriding instead? i.e. have the test derive TestChromiumEnvFoo
> from ChromiumEnvFoo, and override DidSyncDir() and expose directory_syncs() ?

I agree. Yes, I did learn a bit more about gtest's Type-Parameterized Tests and
I may be able to go back to the original implementation and then modify the
test. I'll look into that.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium_win32.cc (right):

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:40: if (errorText[0] != L'\0') {
On 2013/12/12 19:03:14, jsbell wrote:
> Is there a reason to expect trailing whitespace here? (i.e. does a newline
> typically get appended or something?) If so, can you add a comment?
> 
> (I'd feel more comfortable with the "trim" operation occurring on the
> std::string after the conversion myself - maybe implement a separate static
> Trim(std::string*) helper?)

Yes, FormatMessage adds CR/LF :-(, but you are correct - I should have used
TrimWhitespace. Adding that as well as a comment.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:56: class
ChromiumSequentialFile: public SequentialFile {
On 2013/12/12 19:03:14, jsbell wrote:
> I'm slightly nervous that the re-use of this name (ChromiumSequentialFile)
> between the win32 and other env will lead to confusion. They're scoped by the
> anonymous namespace in their respective compilation units so it's not a
> technical issue, but may confuse someone grepping/debugging the code.
> 
> I don't feel strongly about it though.

Good point - I'll change it. Will do the same for ChromiumRandomAccessFile.

https://codereview.chromium.org/113373002/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium_win32.cc:274: #if defined(OS_POSIX)
On 2013/12/12 19:03:14, jsbell wrote:
> How could this ever be true?
> 
> We may want to leave a comment in this no-op method that Windows doesn't have
> file limits in the same way as "POSIX"

I agree. I've deleted the #ifdef and added a comment.

[cmumford, 2013-12-12 23:06:35]

On 2013/12/12 19:03:13, jsbell wrote:
> Sorry for the very scattered feedback. It's taking a while for things to soak
> in.
> 
> I suggest splitting the patch to remove the command line flags for now, so
that
> the change is just the env implementation and test support, even if there's no
> way to turn it on. We'll probably want other reviewers the flags/finch
> settings/etc anyway.
>

If I split them into two commits (which can be done easily) then this means that
the Windows environment has no unit/perf testing unless we hard code the
Windows environment on (which may not be a good first step). My goal was to
(after
this change lands) modify the builders to test the windows environment in
addition
to the default stdio environment.

I can do this if you want - let me know how you want to proceed.

[jsbell, 2013-12-13 23:07:50]

Oops, meant to send this nit yesterday:

https://codereview.chromium.org/113373002/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc
(right):

https://codereview.chromium.org/113373002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc:14:
#include "third_party/leveldatabase/env_chromium_stdio.h"
This should still import env_chromium.h too - in both Chromium and Blink we try
not to rely on transitive includes and explicitly include (or forward declare)
everything used in the compilation unit.

[cmumford, 2013-12-30 21:20:06]

https://codereview.chromium.org/113373002/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc
(right):

https://codereview.chromium.org/113373002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc:14:
#include "third_party/leveldatabase/env_chromium_stdio.h"
On 2013/12/13 23:07:50, jsbell wrote:
> This should still import env_chromium.h too - in both Chromium and Blink we
try
> not to rely on transitive includes and explicitly include (or forward declare)
> everything used in the compilation unit.

OK. It was changed because MakeIOError (the one that took stderr) was moved into
env_chromium_posix.cc. I moved it back.

[dgrogan, 2014-01-07 01:38:41]

Are you waiting for approval of this CL or do you have outstanding local changes
that you are planning to upload?

https://codereview.chromium.org/113373002/diff/70001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium_unittest.cc (right):

https://codereview.chromium.org/113373002/diff/70001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium_unittest.cc:112:
TYPED_TEST(ChromiumEnvMultiPlatformTests, DirectorySyncing) {
Why not do the rest of the tests in this file as TYPED_TESTs?

[cmumford, 2014-01-07 18:43:37]

David:

I have no outstanding local changes, and am waiting for the final LGTM. Please
let me know if I missed any review requests for further changes - I think I
addressed them all.

https://codereview.chromium.org/113373002/diff/70001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium_unittest.cc (right):

https://codereview.chromium.org/113373002/diff/70001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium_unittest.cc:112:
TYPED_TEST(ChromiumEnvMultiPlatformTests, DirectorySyncing) {
On 2014/01/07 01:38:41, dgrogan wrote:
> Why not do the rest of the tests in this file as TYPED_TESTs?

Not strictly necessary as this was the only explicit use of ChromiumEnv.
However, as you know, either ChromiumEnvStdio or ChromiumEnvWin will be
instantiated behind the scenes by IDB::Env(), and the remaining tests will only
test the default one. My plans for running these tests on both platforms was to
have command-line switches, and to have the tests run twice (once for each
platform).

We discussed this and felt it was too much effort for the benefit as Windows
will always test the Win32 API, and the other platforms will test Stdio.

[dgrogan, 2014-01-08 00:54:56]

lgtm

[commit-bot: I haz the power, 2014-01-16 03:33:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cmumford@chromium.org/113373002/140001

[commit-bot: I haz the power, 2014-01-16 05:20:30]

Change committed as 245135

[cmumford, 2014-01-16 18:36:54]

A revert of this CL has been created in
https://codereview.chromium.org/140923005/ by cmumford@chromium.org.

The reason for reverting is: The indexeddb_perf tests on all Windows testers
regressed in performance significantly. Don't yet know the exact cause, but
because the performance drop is significant will revert this change first, and
then proceed with the investigation..

[cmumford, 2014-01-17 19:20:42]

I fixed the performance issue. I should have never been calling FlushFileBuffers
in ChromiumWritableFile::Flush(). I verified that the Windows version of
fflush() only flushes out the stream buffers (writing them using WriteFile,
etc.). The only time it calls FlushFileBuffers is when the file is opened with
the "commit flag" which is a POSIX extension, and defaults to false.

I tried to verify the performance of these changes. You can see these results at
http://cmumford.mtv.corp.google.com/leveldb_win_fix.html

The first five columns are the trunk before my original Windows changes
(r245135) landed. The next two are a debug build with r245135, and the next two
are a release build with r245135. The final two columns are with this change.

[cmumford, 2014-01-27 03:32:23]

ping: still needing lgtm

[dgrogan, 2014-01-27 16:33:11]

lgtm

[commit-bot: I haz the power, 2014-01-27 16:45:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cmumford@chromium.org/113373002/490001

[commit-bot: I haz the power, 2014-01-27 19:34:21]

Change committed as 247279