7 ms·

Author here, if anyone has any questions in relation to me or Sublime HQ please feel free to ask.

evozer 8y ago
Did you consider emulating mmap yourselves? "Memory mapped files work by mapping the full file into a virtual address space and then using page faults to determine which chunks to load into physical memory. In essence it allows you to access the file as if you had read the whole thing into memory, without actually doing so." I feel like this could be done in c++ directly, by maintaining an internal cache for each file that keeps track of which parts of the file are loaded and uses read() to load chunks on demand. Error handling would be a lot simpler (no signals, just a failed read()) and there would be less OS-specific code.
- nneonneo 8y ago
  Well, then you have to implement some kind of plan for efficient caching - some kind of LRU scheme, for example, to prevent the cache from ballooning to unusable sizes - at which point you're reinventing the kernel page cache (poorly). mmap does have a big advantage here if you really need a lot of random accesses.
- maxlybbert 8y ago
  It’s easy enough to read a file in chunks, parsing out the information as you go. This limits memory use as long as you release the chunks when you no longer need them. The operating system can swap out memory as-needed, even if you didn’t get the memory from mmap, so it’s irrelevant where you store the parsed data. Unless you actually need to read the file multiple times (compared to looking at the parsed in-memory data multiple times), this should be fast enough.
- ben-schaaf 8y ago
  This is essentially how databases like PostgreSQL work, but in essence it only avoids the sys-call overhead. The OS is already caching the file, regardless of mmap, so using pread would have likely been enough for us. It totally would have been simpler overall, but each incremental step we made was significantly less work than the refactoring required for pread.
  - quotemstr 8y ago
    > The OS is already caching the file Not necessarily. With O_DIRECT, pread() doesn't put pages into page cache: it just DMAs them directly into your process. Using O_DIRECT and the process-private caching we've been discussing, sophisticated programs (like databases) can (and do!) implement their own "page cache" systems. And because databases have access pattern information that the generic kernel VM subsystem doesn't, such a database can frequently do a better job doing this caching on its own.
    - ben-schaaf 8y ago
      I might have undersold the performance advantage of writing your own cache, but let me reiterate the point I was trying to make: The reason we didn't consider doing so was because we weren't having a performance issue. Writing our own cache would be strictly more work than just using pread and accomplished the same thing.
      - quotemstr 8y ago
        Yeah. For your application, you did the right thing. I was speaking more abstractly.
  - btilly 8y ago
    It totally would have been simpler overall, but each incremental step we made was significantly less work than the refactoring required for pread. Question. In 10 years will you be saying this about the next incremental problem that you run into? If you think this likely, then the next incremental problem is an excuse to do it right.
    - ben509 8y ago
      Mmap is right, though. Pread would also be right. There's a tradeoff and the complexity argument would only win if they knew all this when they started.
    - ben-schaaf 8y ago
emilfihlman 8y ago
Just use signalfd and refactor into an event loop without jumping around.
- nneonneo 8y ago
  This does not work - signalfd cannot catch synchronous signals, per the man page: > Limitations > The signalfd mechanism can't be used to receive signals that are synchronously generated, such as the SIGSEGV signal This is because synchronous signals are fired at the thread that caused them, and signalfd read() calls can't be used to read signals fired at other threads.
eeZah7Ux 8y ago
"In hindsight it's difficult to justify using mmap over pread" This needs a stronger justification. mmap allows reading and writing large data structures without copying, which can be a huge benefit depending on the use case.
- maxlybbert 8y ago
  I think it’s reasonable to say “it’s difficult to justify using mmap when other programs are expected to manipulate, and potentially even delete, the file while you are working with it.” But, honestly, that case will always be hard to handle, and mmap doesn’t make it any worse.
  - nneonneo 8y ago
    I think the problem here is that mmap does make it worse - with (p)read, if you get an errno, you flag an error. with mmap, you have to handle a signal and jump back to an appropriate point in the code (and you can't do this in a cross-platform way), and then flag an error. Obviously, what the upper levels do with the error still has to be worked out, but you can hardly argue that sigsetjmp/siglongjmp + signal handlers + Windows SEH handlers is no worse than "ret = read(...); if (ret < 0) ERROR;"
    - maxlybbert 8y ago
      If the file changes out from under you, all you can do is reread the file. mmap gives you a pointer that you can read from, but you still have to loop through the file’s bytes. I’m pretty sure most programmers are capable of writing a signal handler that sets a flag (volatile sig_atomic_t) for “parsing failed,” and a loop that checks that flag in addition to checking whether the loop is finished for other reasons. Signal handling doesn’t have to be complicated.
      - nneonneo 8y ago
        It’s not quite as simple as setting a flag - if the signal is SIGBUS/SIGSEGV then returning from the handler normally will retry the faulting instruction. So, you either have to map dummy memory into place so the fault doesn’t reoccur, or use sigsetjmp/siglongjmp to jump out to a previously configured exception handler (as Sublime Text did here. I think you’ve mixed up synchronous signals - SEGV, BUS, ABRT - with asynchronous signals like QUIT, INT, USR1. Asynchronous signals can be handled easily with a flag and a loop as you mentioned; synchronous signals are much trickier and much more complicated.
T3OU-736 8y ago
Thank you for the write-up. As an occasional scripter (a coder I am not), I found it very useful - a very nice and structured presentation of both coding practices, and of the challenges of supporting multiple environments with their own way of doing things. Showing the scope of change within the editor is a rather nice touch. Visualization of complexity, if you will.
Hackbraten 8y ago
Small side note for completeness: The effects of oh-noes-my-file-is-gone can be somewhat mitigated by using the heuristics built into NSData (instead of using mmap directly). For example, you call NSData’s `dataWithContentsOfFile:options:error:` with the `NSDataReadingMappedIfSafe` option [1]. The framework will then transparently mmap the file unless it believes there’s an elevated risk of the file going away. Apple doesn’t disclose how NSData exactly makes that decision; however, I’ve found a few reports that say it uses mmap internally when the file is on the root filesystem, and fall back on an in-memory copy otherwise. It’s a rather dumb heuristics though, and may not solve the issue entirely. [1] https://developer.apple.com/documentation/foundation/nsdatareadingoptions/nsdatareadingmappedifsafe https://developer.apple.com/documentation/foundation/nsdatar...
- saagarjha 8y ago
  The method also checks for things like whether the file is in /dev and whether filesystem compression is enabled.
- ben-schaaf 8y ago
  Wouldn't that mean that networked files would be loaded entirely into RAM, negating the whole reason we started using mmap in the first place?
yoodenvranx 8y ago
Since I am a really big fan of Sublime Text I tried to switch from Gitkraken to Sublime Merge a few times but I just don't enjoy working with Sublime Merge. Is there any chance that merging and rebasing via drag-n-drop is coming to Sublime Merge? For me that's the one big feature which keeps me from switching from Gitkraken to Sublime Merge.
daemin 8y ago
I was using Sublime Merge on the weekend and after doing a few changes with my remotes using the normal command line, it exited (well crashed I assume). When things like this happen do you automatically get an error report? Because you mentioned that you use the error reporting library from Google in that article.
- ben-schaaf 8y ago
  We do get crash reports. If you have some more details in relation to this it would be great to hear from you on our forums or on the issue tracker: https://github.com/SublimeTextIssues/Core https://github.com/SublimeTextIssues/Core
  - gianpaj 8y ago
    I believe the correct link for SublimeMerge GH issues is: https://github.com/sublimehq/sublime_merge https://github.com/sublimehq/sublime_merge
avar 8y ago
Is there a post where it's covered why Sublime Merge implements things like packfile reading on its own, rather than using git's own plumbing? E.g. in this case presumably keeping a "git cat-file --batch" would do the trick. I contribute to git.git, and it would be interesting to know if there's inherent issues stopping you from doing that, or if it's implementation problems in some cases (e.g. missing plumbing commands or features). There's definitely interest from upstream in reviewing patches / helping if there's missing or inadequate plumbing. You'd get upstream features for free as they come along. E.g. presumably you haven't implemented the new MIDX format, but that speeds up pack file access by a lot for some use-cases, and presumably the boring bits of low-level git operations aren't much of a selling point in and of themselves. Aside from whether you'd use "git" itself, such a trick of using a slave process you'd talk to over IPC of some sort would cover some of the issues you wrote about, e.g. issue with sharing global state with libraries like Breakpad.
- loeg 8y ago
  The license of git (GPL2) might be an issue for a commercial product. libgit2 is also GPL. (Also, IPC and fork+exec has overhead that mmap or thread in the same program does not.)
  - avar 8y ago
    There are no license issues with bundling the git command-line tools in a commercial product, there's multiple existing proprietary commercial applications built on top of git that do so. The libgit2 code is GPL with a linking exception, so you can use it (unlike "git" itself) as a C library in a proprietary commercial product. > IPC and fork+exec has overhead[...] The "git cat-file --batch" command is something you'd invoke once, and then as your program runs you keep feeding it SHA-1s on stdin and it spews out their content on stdout. So even on Windows the overhead of that should be fine. It's clear from ben-schaaf's other comments (which I read later) that one concern was the simplicity of downstream APIs being able to read the data using a normal C variable. But that just leaves more questions. People in this thread are mentioning pack files, assuming that a multi-GB "git object" must be in a pack, but I notice the original post doesn't say anything about it. If they're reading packs with this they'll need to parse it, resolve deltas etc. So likely the code that deals with the mmap()'d variable is small in any sane codebase (they're surely not doing delta resolution repeatedly all over the place...). If they're very large loose objects those will most likely be zlib compressed, so wouldn't this need to go through some intermediary API layer anyway? I guess if SM itself is adding them it could add them uncompressed. Since ben-schaaf mentioned this not being about performance, but about saving memory I thought this might be something like wanting to extract a small part of a 1GB object from git for display. That seems like a thing an editor might want to do. In that case "git cat-file --batch" would suck, but not for some intrinsic reason. An API could be added that could take the start/end of an object to print out.
jlokier 8y ago
You've used thread_local for the sigjmp_buf. Are you sure the mechanism used by thread_local is safe to use in a signal handler? Relevant bug from Rust: "TLS accesses aren't async-signal-safe", https://github.com/rust-lang/rust/issues/43146 https://github.com/rust-lang/rust/issues/43146 I'm thinking even if it works on certain OSes, it's not guaranteed, because a signal handler's context is not a thread context - or is it? E.g. the thread_local mechanism might depend on compile-time options (affecting how thread_local, whether it allocates memory on demand, and how it relocates the memory block when loading a shared library), whether it's main program or an -fPIC shared library, and the type of thread library (different ways of implementing pthreads).
- ben-schaaf 8y ago
  Considering we haven't had any mmap related crashes anymore I'd say its safe to assume that its good enough for our application.
chinhodado 8y ago
About Sublime Merge, is there any plan for plugins?
- ben-schaaf 8y ago
  We do want to support plugins at some point, but we don't have a timeline yet.
cbsmith 8y ago
I'm just going to leave this right here: http://man7.org/linux/man-pages/man2/signalfd.2.html http://man7.org/linux/man-pages/man2/signalfd.2.html
- ben-schaaf 8y ago
  I know about signalfd, how would it help dealing with mmap? > The signalfd mechanism can't be used to receive signals that are synchronously generated, such as the SIGSEGV
  - cbsmith 8y ago
    So a common way to handle synchronous signals is via a mechanism much like signalfd, where your signal handler just appends to a pipe that can be read at a convenient time.

Did you consider emulating mmap yourselves? "Memory mapped files work by mapping the full file into a virtual address space and then using page faults to determine which chunks to load into physical memory. In essence it allows you to access the file as if you had read the whole thing into memory, without actually doing so." I feel like this could be done in c++ directly, by maintaining an internal cache for each file that keeps track of which parts of the file are loaded and uses read() to load chunks on demand. Error handling would be a lot simpler (no signals, just a failed read()) and there would be less OS-specific code.

nneonneo 8y ago
Well, then you have to implement some kind of plan for efficient caching - some kind of LRU scheme, for example, to prevent the cache from ballooning to unusable sizes - at which point you're reinventing the kernel page cache (poorly). mmap does have a big advantage here if you really need a lot of random accesses.
maxlybbert 8y ago
It’s easy enough to read a file in chunks, parsing out the information as you go. This limits memory use as long as you release the chunks when you no longer need them. The operating system can swap out memory as-needed, even if you didn’t get the memory from mmap, so it’s irrelevant where you store the parsed data. Unless you actually need to read the file multiple times (compared to looking at the parsed in-memory data multiple times), this should be fast enough.
ben-schaaf 8y ago
This is essentially how databases like PostgreSQL work, but in essence it only avoids the sys-call overhead. The OS is already caching the file, regardless of mmap, so using pread would have likely been enough for us. It totally would have been simpler overall, but each incremental step we made was significantly less work than the refactoring required for pread.
- quotemstr 8y ago
  > The OS is already caching the file Not necessarily. With O_DIRECT, pread() doesn't put pages into page cache: it just DMAs them directly into your process. Using O_DIRECT and the process-private caching we've been discussing, sophisticated programs (like databases) can (and do!) implement their own "page cache" systems. And because databases have access pattern information that the generic kernel VM subsystem doesn't, such a database can frequently do a better job doing this caching on its own.
  - ben-schaaf 8y ago
    I might have undersold the performance advantage of writing your own cache, but let me reiterate the point I was trying to make: The reason we didn't consider doing so was because we weren't having a performance issue. Writing our own cache would be strictly more work than just using pread and accomplished the same thing.
    - quotemstr 8y ago
      Yeah. For your application, you did the right thing. I was speaking more abstractly.
- btilly 8y ago
  It totally would have been simpler overall, but each incremental step we made was significantly less work than the refactoring required for pread. Question. In 10 years will you be saying this about the next incremental problem that you run into? If you think this likely, then the next incremental problem is an excuse to do it right.
  - ben509 8y ago
    Mmap is right, though. Pread would also be right. There's a tradeoff and the complexity argument would only win if they knew all this when they started.
  - ben-schaaf 8y ago

emilfihlman 8y ago

Just use signalfd and refactor into an event loop without jumping around.

nneonneo 8y ago
This does not work - signalfd cannot catch synchronous signals, per the man page: > Limitations > The signalfd mechanism can't be used to receive signals that are synchronously generated, such as the SIGSEGV signal This is because synchronous signals are fired at the thread that caused them, and signalfd read() calls can't be used to read signals fired at other threads.

eeZah7Ux 8y ago

"In hindsight it's difficult to justify using mmap over pread" This needs a stronger justification. mmap allows reading and writing large data structures without copying, which can be a huge benefit depending on the use case.

maxlybbert 8y ago
I think it’s reasonable to say “it’s difficult to justify using mmap when other programs are expected to manipulate, and potentially even delete, the file while you are working with it.” But, honestly, that case will always be hard to handle, and mmap doesn’t make it any worse.
- nneonneo 8y ago
  I think the problem here is that mmap does make it worse - with (p)read, if you get an errno, you flag an error. with mmap, you have to handle a signal and jump back to an appropriate point in the code (and you can't do this in a cross-platform way), and then flag an error. Obviously, what the upper levels do with the error still has to be worked out, but you can hardly argue that sigsetjmp/siglongjmp + signal handlers + Windows SEH handlers is no worse than "ret = read(...); if (ret < 0) ERROR;"
  - maxlybbert 8y ago
    If the file changes out from under you, all you can do is reread the file. mmap gives you a pointer that you can read from, but you still have to loop through the file’s bytes. I’m pretty sure most programmers are capable of writing a signal handler that sets a flag (volatile sig_atomic_t) for “parsing failed,” and a loop that checks that flag in addition to checking whether the loop is finished for other reasons. Signal handling doesn’t have to be complicated.
    - nneonneo 8y ago
      It’s not quite as simple as setting a flag - if the signal is SIGBUS/SIGSEGV then returning from the handler normally will retry the faulting instruction. So, you either have to map dummy memory into place so the fault doesn’t reoccur, or use sigsetjmp/siglongjmp to jump out to a previously configured exception handler (as Sublime Text did here. I think you’ve mixed up synchronous signals - SEGV, BUS, ABRT - with asynchronous signals like QUIT, INT, USR1. Asynchronous signals can be handled easily with a flag and a loop as you mentioned; synchronous signals are much trickier and much more complicated.

T3OU-736 8y ago

Thank you for the write-up. As an occasional scripter (a coder I am not), I found it very useful - a very nice and structured presentation of both coding practices, and of the challenges of supporting multiple environments with their own way of doing things. Showing the scope of change within the editor is a rather nice touch. Visualization of complexity, if you will.

Hackbraten 8y ago

Small side note for completeness: The effects of oh-noes-my-file-is-gone can be somewhat mitigated by using the heuristics built into NSData (instead of using mmap directly). For example, you call NSData’s `dataWithContentsOfFile:options:error:` with the `NSDataReadingMappedIfSafe` option [1]. The framework will then transparently mmap the file unless it believes there’s an elevated risk of the file going away. Apple doesn’t disclose how NSData exactly makes that decision; however, I’ve found a few reports that say it uses mmap internally when the file is on the root filesystem, and fall back on an in-memory copy otherwise. It’s a rather dumb heuristics though, and may not solve the issue entirely. [1] https://developer.apple.com/documentation/foundation/nsdatareadingoptions/nsdatareadingmappedifsafe https://developer.apple.com/documentation/foundation/nsdatar...

saagarjha 8y ago
The method also checks for things like whether the file is in /dev and whether filesystem compression is enabled.
ben-schaaf 8y ago
Wouldn't that mean that networked files would be loaded entirely into RAM, negating the whole reason we started using mmap in the first place?

yoodenvranx 8y ago

Since I am a really big fan of Sublime Text I tried to switch from Gitkraken to Sublime Merge a few times but I just don't enjoy working with Sublime Merge. Is there any chance that merging and rebasing via drag-n-drop is coming to Sublime Merge? For me that's the one big feature which keeps me from switching from Gitkraken to Sublime Merge.

daemin 8y ago

I was using Sublime Merge on the weekend and after doing a few changes with my remotes using the normal command line, it exited (well crashed I assume). When things like this happen do you automatically get an error report? Because you mentioned that you use the error reporting library from Google in that article.

ben-schaaf 8y ago
We do get crash reports. If you have some more details in relation to this it would be great to hear from you on our forums or on the issue tracker: https://github.com/SublimeTextIssues/Core https://github.com/SublimeTextIssues/Core
- gianpaj 8y ago
  I believe the correct link for SublimeMerge GH issues is: https://github.com/sublimehq/sublime_merge https://github.com/sublimehq/sublime_merge

avar 8y ago

Is there a post where it's covered why Sublime Merge implements things like packfile reading on its own, rather than using git's own plumbing? E.g. in this case presumably keeping a "git cat-file --batch" would do the trick. I contribute to git.git, and it would be interesting to know if there's inherent issues stopping you from doing that, or if it's implementation problems in some cases (e.g. missing plumbing commands or features). There's definitely interest from upstream in reviewing patches / helping if there's missing or inadequate plumbing. You'd get upstream features for free as they come along. E.g. presumably you haven't implemented the new MIDX format, but that speeds up pack file access by a lot for some use-cases, and presumably the boring bits of low-level git operations aren't much of a selling point in and of themselves. Aside from whether you'd use "git" itself, such a trick of using a slave process you'd talk to over IPC of some sort would cover some of the issues you wrote about, e.g. issue with sharing global state with libraries like Breakpad.

loeg 8y ago
The license of git (GPL2) might be an issue for a commercial product. libgit2 is also GPL. (Also, IPC and fork+exec has overhead that mmap or thread in the same program does not.)
- avar 8y ago
  There are no license issues with bundling the git command-line tools in a commercial product, there's multiple existing proprietary commercial applications built on top of git that do so. The libgit2 code is GPL with a linking exception, so you can use it (unlike "git" itself) as a C library in a proprietary commercial product. > IPC and fork+exec has overhead[...] The "git cat-file --batch" command is something you'd invoke once, and then as your program runs you keep feeding it SHA-1s on stdin and it spews out their content on stdout. So even on Windows the overhead of that should be fine. It's clear from ben-schaaf's other comments (which I read later) that one concern was the simplicity of downstream APIs being able to read the data using a normal C variable. But that just leaves more questions. People in this thread are mentioning pack files, assuming that a multi-GB "git object" must be in a pack, but I notice the original post doesn't say anything about it. If they're reading packs with this they'll need to parse it, resolve deltas etc. So likely the code that deals with the mmap()'d variable is small in any sane codebase (they're surely not doing delta resolution repeatedly all over the place...). If they're very large loose objects those will most likely be zlib compressed, so wouldn't this need to go through some intermediary API layer anyway? I guess if SM itself is adding them it could add them uncompressed. Since ben-schaaf mentioned this not being about performance, but about saving memory I thought this might be something like wanting to extract a small part of a 1GB object from git for display. That seems like a thing an editor might want to do. In that case "git cat-file --batch" would suck, but not for some intrinsic reason. An API could be added that could take the start/end of an object to print out.

jlokier 8y ago

You've used thread_local for the sigjmp_buf. Are you sure the mechanism used by thread_local is safe to use in a signal handler? Relevant bug from Rust: "TLS accesses aren't async-signal-safe", https://github.com/rust-lang/rust/issues/43146 https://github.com/rust-lang/rust/issues/43146 I'm thinking even if it works on certain OSes, it's not guaranteed, because a signal handler's context is not a thread context - or is it? E.g. the thread_local mechanism might depend on compile-time options (affecting how thread_local, whether it allocates memory on demand, and how it relocates the memory block when loading a shared library), whether it's main program or an -fPIC shared library, and the type of thread library (different ways of implementing pthreads).

ben-schaaf 8y ago
Considering we haven't had any mmap related crashes anymore I'd say its safe to assume that its good enough for our application.

chinhodado 8y ago

About Sublime Merge, is there any plan for plugins?

ben-schaaf 8y ago
We do want to support plugins at some point, but we don't have a timeline yet.

cbsmith 8y ago

I'm just going to leave this right here: http://man7.org/linux/man-pages/man2/signalfd.2.html http://man7.org/linux/man-pages/man2/signalfd.2.html

ben-schaaf 8y ago
I know about signalfd, how would it help dealing with mmap? > The signalfd mechanism can't be used to receive signals that are synchronously generated, such as the SIGSEGV
- cbsmith 8y ago
  So a common way to handle synchronous signals is via a mechanism much like signalfd, where your signal handler just appends to a pipe that can be read at a convenient time.