Add MemMapFileStream. Fixes in memFiles. #7944

data-man · 2018-06-02T08:53:30Z

No description provided.

Varriount · 2018-06-02T09:27:13Z

lib/pure/memfiles.nim

+    result = flushViewOfFile(f.mem, f.size.int32) == 0
+    if not result:
+      let lastErr = osLastError()
+      if lastErr != ERROR_LOCK_VIOLATION.OSErrorCode:


Why is this particular error ignored?

From MSDN:

The long remarks
Remarks
Flushing a range of a mapped view initiates writing of dirty pages within that range to the disk. Dirty pages are those whose contents have changed since the file view was mapped. The FlushViewOfFile function does not flush the file metadata, and it does not wait to return until the changes are flushed from the underlying hardware disk cache and physically written to disk. To flush all the dirty pages plus the metadata for the file and ensure that they are physically written to disk, call FlushViewOfFile and then call the FlushFileBuffers function.

When flushing a memory-mapped file over a network, FlushViewOfFile guarantees that the data has been written from the local computer, but not that the data resides on the remote computer. The server can cache the data on the remote side. Therefore, FlushViewOfFile can return before the data has been physically written to disk.

Oh!
I found this code: MappedByteBuffer.c#L66

retry = 0; do { result = FlushViewOfFile(a, (DWORD)len); if ((result != 0) || (GetLastError() != ERROR_LOCK_VIOLATION)) break; retry++; } while (retry < 3); ``

That code retries the flushing operation a maximum of 3 times. After 3 times, if the lock error occurs, it is still raised.

I see.
Therefore I ignore ERROR_LOCK_VIOLATION.

Maybe add a similar code both for Windows and for Posix?

Varriount · 2018-06-02T09:30:38Z

Please fix the build errors. It appears the code breaks when string tainting mode is turned on.

data-man · 2018-06-02T10:47:34Z

Why bugs in memfiles didn't appear in the tests before?
Caching in CIs?

data-man · 2018-06-03T06:57:25Z

Ready for something.

Araq · 2018-06-03T19:58:13Z

tests/stdlib/tmemmapstreams.nim

+import os, streams
+var
+  mms: MemMapFileStream
+  fn = "test.mmapstream"


Use a const for fn.

Varriount · 2018-06-04T05:02:29Z

lib/pure/memfiles.nim

+    result = flushViewOfFile(f.mem, 0) != 0
+    if not result:
+      let lastErr = osLastError()
+      if lastErr != ERROR_LOCK_VIOLATION.OSErrorCode:


This shouldn't be ignored. The C code this was based on tries a maximum of 3 times iff ERROR_LOCK_VIOLATION is raised each time. The 4th time, ERROR_LOCK_VIOLATION is raised.

So I propose proc flush*(f: var MemFile, triesNumber: int = 3): bool both for Windows and Posix.

Why would that return bool? Nim's stdlib uses exceptions.

You can call flush and if result == false then repeat the call. Can be useful, IMHO.

You might as well catch the exception then. And why would I repeat the call myself when it didn't succeed after triesNumber?

I followed the logic proc open*(f: var File, ...): bool.

So

add triesNumber (or numTries)?

remove bool result?

no raise an exception inside flush?

raise an exception for MemMapFileStream.flush?

proc flush(f: var MemFile; attempts = 3) {.raises: [IOError].}

I see inconsistency here: flush for FileStream don't raises exception.
flushFile has WriteIOEffect tag only.

flushFile is allowed to raise that and should be annotated too.

…stream

Added the attempts parameter

dom96 · 2018-06-06T19:40:38Z

lib/pure/memfiles.nim

@@ -245,6 +249,30 @@ proc open*(filename: string, mode: FileMode = fmRead,
      if close(result.handle) == 0:
        result.handle = -1

+proc flush*(f: var MemFile; attempts: int = 3) =


Why would flushing require multiple attempts?

Ahh, the hidden reviews contain the answers. The original code also contains this really useful comment which should be copied into our code IMO.

dom96 · 2018-06-06T19:41:06Z

lib/pure/streams.nim

@@ -437,6 +438,71 @@ when not defined(js):
    else:
      raise newEIO("cannot open file")

+  type
+    MemMapFileStream* = ref MemMapFileStreamObj ## a stream that encapsulates a `MemFile`


This should be in the memfiles module.

And StringStream in the strutils?

No because string is in system.

So, move to memfiles?

Yeah, please.

dom96 · 2018-06-06T19:50:01Z

lib/pure/memfiles.nim

+    var res = false
+    while retry <= attempts:
+      res = flushViewOfFile(f.mem, 0) != 0
+      if res:


Also, you're retrying no matter what the error is, that's bad. Only retry on the ERROR_LOCK_VIOLATION error!

I don't see why replicating this to POSIX makes sense. In fact, I'm skeptical about whether this should be configurable via a param at all.

Araq · 2018-06-06T20:41:15Z

lib/pure/memfiles.nim

+  ## Flushes `f`'s buffer for the number of attempts equal to `attempts`.
+  ## If were errors an exception `OSError` will be raised.
+  when defined(windows):
+    var retry = 0


Are for loops outdated now?

Oops, С-thinking. :)

data-man · 2018-06-06T21:09:52Z

All a bit complicated. :)
Linux has int sync_file_range(int fd, off64_t offset, off64_t nbytes, unsigned int flags).
It's can be more efficient for Linux only (we haven't it in linux.nim).

dom96 · 2018-06-07T09:06:05Z

You're still not checking the error code on Windows and are performing the retries on Linux.

#7944 (comment)

data-man · 2018-06-07T09:43:22Z

Yes, and now it's correct, IMO.

are performing the retries on Linux.

Why not?

Araq · 2018-06-08T15:36:58Z

lib/pure/memfiles.nim

+  when defined(windows):
+    var res = false
+    for i in 1..attempts:
+      res = flushViewOfFile(f.mem, 0) != 0


Again, what dom said, why not base the retry on the return value of flushViewOfFile?

Currently, an exception will be raised due to any OS error.
The developer has to decide what to do next.

Ignoring some specific errors, it's a bad design.

No, it's retrying only on specific errors, it's good design. Of course you should raise an exception for any other error.

Well, then I need to investigate/experiments with flags and error codes of msync, too. :)

Araq · 2018-06-08T15:37:11Z

lib/pure/memfiles.nim

@@ -245,6 +249,26 @@ proc open*(filename: string, mode: FileMode = fmRead,
      if close(result.handle) == 0:
        result.handle = -1

+proc flush*(f: var MemFile; attempts: uint = 3) =


Use Natural instead of uint here.

dom96 · 2018-06-08T20:04:19Z

are performing the retries on Linux.

Why not?

These retries are only for a specific Windows-only error. They do not apply to Linux.

data-man · 2018-06-08T20:38:26Z

They do not apply to Linux.

To be more precise, msync can returns EBUSY if MS_INVALIDATE was specified in the flags.

(Since Linux 2.6.19, MS_ASYNC is in fact a no-op, since the kernel properly tracks
dirty pages and flushes them to storage as necessary.)

dom96 · 2018-06-08T21:02:33Z

Okay, so if it's appropriate to retry on that error code then do the retry, but don't retry on any error.

data-man · 2018-06-08T21:17:45Z

Maybe to do a pause between attempts?
I tried use sleep from os module, but it has TimeEffect tag.
What if to use getTicks from system/timers?

Araq · 2018-06-14T10:06:02Z

Maybe to do a pause between attempts?

No.

data-man · 2018-06-14T14:48:15Z

Ready for merge, I hope.

dom96 · 2018-06-14T16:34:37Z

Good enough I suppose :)

data-man added 2 commits June 2, 2018 11:40

Add MemMapFileStream

f2c371f

Added tests

02c2ae1

Varriount reviewed Jun 2, 2018

View reviewed changes

Fixed bug in memfiles (zero index for string)

6732bb9

data-man changed the title ~~Add MemMapFileStream~~ Add MemMapFileStream. Fixes in memFiles. Jun 2, 2018

data-man added 7 commits June 2, 2018 15:43

Added flush to changelog

bb17a88

Attempt to fix Win's nuances

7855833

Fix attempt to fix

f2bad32

Continue...

092287b

And again...

8b0dc0f

Reworked tests (all for win on Win)

c0ac817

Fixes in flush (Win)

432a768

Araq reviewed Jun 3, 2018

View reviewed changes

tests/stdlib/tmemmapstreams.nim Outdated

import os, streams

var

mms: MemMapFileStream

fn = "test.mmapstream"

Copy link

Member

Araq Jun 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a const for fn.

Replace fn vars to consts

3b684ab

Varriount previously requested changes Jun 4, 2018

View reviewed changes

data-man added 3 commits June 6, 2018 17:26

Update changelog

b512728

Added the attempts parameter to the flush

2e7ae6d

Merge branch 'devel' of https://github.com/nim-lang/Nim into mem_map_…

6dd0f4e

…stream

dom96 requested changes Jun 6, 2018

View reviewed changes

dom96 reviewed Jun 6, 2018

View reviewed changes

Araq reviewed Jun 6, 2018

View reviewed changes

data-man added 2 commits June 7, 2018 01:10

Replace while to for

6f0c022

Move to memfiles

8c6558f

Araq reviewed Jun 8, 2018

View reviewed changes

Use Natural instead of uint

5420c03

data-man added 2 commits June 14, 2018 15:34

Update changelog

d0abf77

Better error messages for append mode. Handle specific cases.

8629eb7

dom96 merged commit bf5d619 into nim-lang:devel Jun 14, 2018

Add MemMapFileStream. Fixes in memFiles. #7944

Add MemMapFileStream. Fixes in memFiles. #7944

Conversation

data-man commented Jun 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Varriount commented Jun 2, 2018

data-man commented Jun 2, 2018

data-man commented Jun 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

data-man commented Jun 6, 2018

dom96 commented Jun 7, 2018

data-man commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dom96 commented Jun 8, 2018

data-man commented Jun 8, 2018

dom96 commented Jun 8, 2018

data-man commented Jun 8, 2018

Araq commented Jun 14, 2018

data-man commented Jun 14, 2018

dom96 commented Jun 14, 2018