Let sqlite use fdatasync() on Linux-based systems

Let sqlite use fdatasync() on Linux-based systems

fdatasync() should be faster than fsync().

From sqlite3.c:
/*
** We do not trust systems to provide a working fdatasync().  Some do.
** Others do no.  To be safe, we will stick with the (slightly slower)
** fsync(). If you know that your system does support fdatasync() correctly,
** then simply compile with -Dfdatasync=fdatasync
*/

BUG=469071
Committed: https://crrev.com/195310ab0b38de5704b9fa7ec9a804d68d3cdb88
Cr-Commit-Position: refs/heads/master@{#322679}

[hashimoto, 2015-03-26 09:57:44]

hashimoto@chromium.org changed reviewers:
+ shess@chromium.org

[Scott Hess - ex-Googler, 2015-03-26 17:46:00]

Ha!  I was also writing this CL yesterday afternoon.

Does "linux" cover ChromiumOS?

Did you verify that this actually made the compile-time change expected?  I
don't think it's plausible to write an actual test for, I was thinking more
along the lines of putting an #else and #error in sqlite3.c at the fdatasync
define to prove things for linux, android, and chromeos.

https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.gyp
File third_party/sqlite/sqlite.gyp (right):

https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.g...
third_party/sqlite/sqlite.gyp:70: }],
I couldn't figure out if this should be here...

https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.g...
third_party/sqlite/sqlite.gyp:141: }],
Or here,

https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.g...
third_party/sqlite/sqlite.gyp:155: 'SQLITE_DEFAULT_FILE_FORMAT=4',
and here.

[Scott Hess - ex-Googler, 2015-03-26 21:51:07]

I still wonder about chromeos in the gyp case, if you know.

https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.gyp
File third_party/sqlite/sqlite.gyp (right):

https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.g...
third_party/sqlite/sqlite.gyp:70: }],
On 2015/03/26 17:46:00, Scott Hess wrote:
> I couldn't figure out if this should be here...

I was updating my Linux client, so I patched your CL in and on the
straight-Linux client, this doesn't hit the define, and strace shows fdatasync
being called, so this works.

And since I don't know where it should be, and GN will hopefully remove the need
for this file, I guess this is fine.

[hashimoto, 2015-03-27 05:14:57]

On 2015/03/26 17:46:00, Scott Hess wrote:
> Ha!  I was also writing this CL yesterday afternoon.
Wow!
Then please feel free to ignore this CL and submit your own if you find it more
convenient :)
> 
> Does "linux" cover ChromiumOS?
Yes, Chromium OS is os=="linux" and chromeos==1.
Though, it seems this change doesn't affect Chromium OS in practice because
SQLITE_NO_SYNC is defined in sqlite.gyp.
> 
> Did you verify that this actually made the compile-time change expected?  I
> don't think it's plausible to write an actual test for, I was thinking more
> along the lines of putting an #else and #error in sqlite3.c at the fdatasync
> define to prove things for linux, android, and chromeos.
Yes, I verified this results in a compile-time change manually (with
GYP_DEFINES="chromeos=1" Linux build), and couldn't come up with an automated
test for it.

Probably it might be better to replace "#if !defined(fdatasync)" in sqlite code
with "#if !defined(fdatasync) && !defined(OS_LINUX) && !defined(OS_ANDROID)".
> 
>
https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.gyp
> File third_party/sqlite/sqlite.gyp (right):
> 
>
https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.g...
> third_party/sqlite/sqlite.gyp:70: }],
> I couldn't figure out if this should be here...
> 
>
https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.g...
> third_party/sqlite/sqlite.gyp:141: }],
> Or here,
> 
>
https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.g...
> third_party/sqlite/sqlite.gyp:155: 'SQLITE_DEFAULT_FILE_FORMAT=4',
> and here.

[hashimoto, 2015-03-27 05:15:02]

https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.gyp
File third_party/sqlite/sqlite.gyp (right):

https://codereview.chromium.org/1035903002/diff/1/third_party/sqlite/sqlite.g...
third_party/sqlite/sqlite.gyp:70: }],
On 2015/03/26 21:51:07, Scott Hess wrote:
> On 2015/03/26 17:46:00, Scott Hess wrote:
> > I couldn't figure out if this should be here...
> 
> I was updating my Linux client, so I patched your CL in and on the
> straight-Linux client, this doesn't hit the define, and strace shows fdatasync
> being called, so this works.
> 
> And since I don't know where it should be, and GN will hopefully remove the
need
> for this file, I guess this is fine.

I chose this place because this |fdatasync| is used in the same place as
SQLITE_NO_SYNC (namely, full_fsync() function in sqlite3.c).

[Scott Hess - ex-Googler, 2015-03-27 14:13:11]

lgtm.

I'm happy to have someone else to ask questions of, since my main worries were
that I'd add the define but it wouldn't actually change the code or something...

You're right on the ChromiumOS no-sync thing.  I'm surprised I missed that,
because I was being annoyed by it recently (it was a short-sighted decision).

[Scott Hess - ex-Googler, 2015-03-27 22:51:17]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2015-03-27 22:51:35]

On 2015/03/27 14:13:11, Scott Hess wrote:
> lgtm.
> 
> I'm happy to have someone else to ask questions of, since my main worries were
> that I'd add the define but it wouldn't actually change the code or
something...
> 
> You're right on the ChromiumOS no-sync thing.  I'm surprised I missed that,
> because I was being annoyed by it recently (it was a short-sighted decision).

Realized you're in a different timezone, so I figure I'll punch the commit
button.

[commit-bot: I haz the power, 2015-03-27 22:52:01]

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

[commit-bot: I haz the power, 2015-03-27 23:39:26]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-27 23:40:08]

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

[hashimoto, 2015-03-28 07:42:02]

On 2015/03/27 22:51:35, Scott Hess wrote:
> On 2015/03/27 14:13:11, Scott Hess wrote:
> > lgtm.
> > 
> > I'm happy to have someone else to ask questions of, since my main worries
were
> > that I'd add the define but it wouldn't actually change the code or
> something...
> > 
> > You're right on the ChromiumOS no-sync thing.  I'm surprised I missed that,
> > because I was being annoyed by it recently (it was a short-sighted
decision).
> 
> Realized you're in a different timezone, so I figure I'll punch the commit
> button.

Thanks!