Disable mmap.Open for current datafile #239

Open
opened 4 weeks ago by jason3gb · 4 comments
jason3gb commented 4 weeks ago

Hi, after read through the source code, I'm a little confused about why we need to mmap the current datafile to memory, even though we never use the mmap reader when the datafile is not readonly.

Then only usage of the mmap reader is at the datafile.ReadAt()


	if df.w == nil {
		n, err = df.ra.ReadAt(b, index)
	} else {
		n, err = df.r.ReadAt(b, index)
	}

But we create a mmap Reader everytime here in the Newdatafile()


	ra, err = mmap.Open(fn)
	if err != nil {
		return nil, err
	}


Hi, after read through the source code, I'm a little confused about why we need to mmap the current datafile to memory, even though we never use the mmap reader when the datafile is not readonly. Then only usage of the mmap reader is at the datafile.ReadAt() ```golang if df.w == nil { n, err = df.ra.ReadAt(b, index) } else { n, err = df.r.ReadAt(b, index) } ``` But we create a mmap Reader everytime here in the Newdatafile() ```golang ra, err = mmap.Open(fn) if err != nil { return nil, err } ```
Owner

Hmmm thanks for reporting this, I'll have a close look! The intent behind memory mapping the datafiles was for performance

Hmmm thanks for reporting this, I'll have a close look! The intent behind memory mapping the datafiles was for performance
Poster

Yeah, I figured. It just seems that the mmap reader is never used when the datafile is the current datafile, which is not readonly. Since the mmap package using the mmap with PROT_READ option, the mmap reader is meant for readonly purpose. However, for the current datafile, the read/write option is always through the fd got from the Open syscall, so I think it might be better not set the ra for the current datafile.

Yeah, I figured. It just seems that the mmap reader is never used when the datafile is the current datafile, which is not readonly. Since the mmap package using the mmap with PROT_READ option, the mmap reader is meant for readonly purpose. However, for the current datafile, the read/write option is always through the fd got from the Open syscall, so I think it might be better not set the ra for the current datafile.
Owner

Yeah I think you're right actually. Did you want to put a PR up for this since you discovered it? 🤔 You should get the credit for fixing this 🤗

Yeah I _think_ you're right actually. Did you want to put a PR up for this since you discovered it? 🤔 You should get the credit for fixing this 🤗
Poster

Thanks, I'll give it a try.

Thanks, I'll give it a try.
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.