< gmaxwell>
again: we can stay with select on windows. It doesn't have the 1024 FD problem.
< ossifrage>
The fix for my specific problem was to just modify how many mmaps() leveldb will make
< gmaxwell>
ossifrage: do you have any idea why the number of mmaps would be limited at all, on 64 bit systems?
< ossifrage>
The comment is about "performance reasons" for large databases... But 1000 mmaps is in the noise I think
< ossifrage>
I changed src/leveldb/util/env_posix.cc mmap_limit from 1000 to 4096
< gmaxwell>
I don't understand what they mean there.. are they thinking in terms of TLB load or something?
< ossifrage>
mmap() is a great way to generate a very large amount of write pressure, but it seems like most of the leveldb use in bitcoin has a very low change rate
< ossifrage>
But lots of memory and slow IO will do that just fine without a single mmmap()
< gmaxwell>
leveldb's writes are very structured, basically it's an append only thing, that periodically rewrites whole files.
< sipa>
mmap is only used for readonly things
< sipa>
afaik
< sipa>
the files are produced in one go, by dumping a sorted table to disk
< gmaxwell>
I wish the leveldb project were more active, it would be nice if we could ask if there is a reason we shouldn't just kill the limit on 64-bit.
< ossifrage>
sipa, that is a good idea, especially if the writes are streaming
< phantomcircuit>
sipa, gmaxwell should be used only for reads
< phantomcircuit>
in which case increasing the limit to... infinity shouldn't be an issue on 64bit systems
< gmaxwell>
doesn't replace using poll instead of select.
< gmaxwell>
phantomcircuit: hows the PR coming? :P
< phantomcircuit>
well i had one that looked like it worked but then fucking wsapoll is broken
< phantomcircuit>
soooo
< phantomcircuit>
try again
< sipa>
phantomcircuit: don't do wsapoll
< sipa>
just poll on sane OSes
< sipa>
keep using select on windows
< gmaxwell>
the main reason to not use select is the stupid fd value limit, but that doesn't apply for windows.
< phantomcircuit>
i mean yeah but that's actually a bigger change
< gmaxwell>
it's just "keep the existing code, add the poll in an ifdef", no
< phantomcircuit>
gmaxwell, it's slightly different, there's no FD_ISSET
< phantomcircuit>
you iterate through a list of fd, events pairs
< phantomcircuit>
the select logic iterates over all the nodes and calls FD_ISSET
< phantomcircuit>
(which iirc is insane cause FD_ISSET iterates over all the fds)
< sipa>
phantomcircuit: in windows it does
< sipa>
in linux it's a bitfield test
< phantomcircuit>
sipa, oh
< phantomcircuit>
well either way there isn't a trivial way to do that mapping with poll()
< sipa>
phantomcircuit: which is btw the reason why select is resteicted to fd's below 1024
< sipa>
fdset id a 128 byte array
< sipa>
*is
< phantomcircuit>
yeah makes sense
< phantomcircuit>
i'll add the iteration needs to be reversed for epoll or kqueue also anyways
< fanquake>
cfields Looking forward to the turtles!
< cfields>
fanquake: heh, I just pushed it so that dongcarl can get his hands dirty. It's still an absolute disaster.
< phantomcircuit>
cfields, is there a map from fd to CNode ?
< cfields>
phantomcircuit: don't believe so. IIRC we always just iterate.
< phantomcircuit>
how does that work with libevent stuff? iirc it's just calling a callback with the fd right?
< cfields>
phantomcircuit: the libevent stuff hasn't been merged. You mean in my branches?
< phantomcircuit>
yeah
< cfields>
anyway, yea, callback with fd and a few other things, and a caller-supplied pointer
< phantomcircuit>
oh i see the caller supplied pointer
< phantomcircuit>
right so libevent is basically keeping that map for you
< cfields>
well everything's done in reverse, so there shouldn't be any need to ever lookup an fd
< cfields>
so, i suppose :)
< phantomcircuit>
cfields, well the underlying epoll thing requires you can map fd to cnode
< phantomcircuit>
just with libevent it's doing it for you implicitly with the callback data
< phantomcircuit>
for poll() you need the same but it's explicit
< cfields>
I thought you had to iterate through the fd list anyway with poll similar to select. Am I completely misremembering?
< cfields>
after it wakes for active fds, I mean.
< phantomcircuit>
cfields, you iterate through the list of fd's you gave it
< phantomcircuit>
yes
< cfields>
phantomcircuit: right, so why the need for a map? You've got the pointers to the CNodes that you pulled the fds from, and you need to test them anyway
< cfields>
or are you just trying to eliminate the overhead of the iteration of nodes that didn't wake?
< cfields>
(I don't remember if poll gives you anything to help avoid that)
< phantomcircuit>
cfields, i mean i can make the map right there, but it's awkward
< cfields>
phantomcircuit: good luck :)
< cfields>
nnite
< wumpus>
kallewoof: if there is no non-locale-independent function, you're going to have to implement one yourself
< wumpus>
kallewoof: usually this is trivial as the ASCII case of the string functions is trivial
< kallewoof>
wumpus: ok
< jonasschnelli>
who own drahtbot?
< jonasschnelli>
*owns
< kallewoof>
jonasschnelli: MarcoFalke
< jonasschnelli>
Nice work... I wasn't aware that it does also builds via gitian
< wumpus>
yes it's a great bot
< fanquake>
^ It keeps getting better
< fanquake>
I wonder if it'll be running/posting the results of some of the benchmarks from perfmonitor
< wumpus>
it's not surprising that it makes running the functional tests fractially slower, as test cases were added, but 10% is a lot
< provoostenator>
xpub derivation is expensive
< provoostenator>
You could mock the actual derivation with lookup tables.
< provoostenator>
But that's pretty invasive for the functional test suite.
< wumpus>
yes but what I mean is that the test framework consists of so many tests, this mean that one test takes up ~1/10th of it
< provoostenator>
That's not surprising because of how many derivations it's doing, trying to scan up to a thousand (?) keys for those xpub/* patterns. Using the range param for scantxoutset might help.
< wumpus>
good point, yes true
< provoostenator>
I left a note for sipa on the PR. The default is 1000 and sevearl tests use 1499, I suggest using 1 and 2.
< wumpus>
thanks
< provoostenator>
fanquake: no rush, but how often does that bot build things? There's only one ticket with a "Needs gitian build" label atm.
< fanquake>
provoostenator: Not 100% sure if it's completely automated or not. MarcoFalke should be able to help out.
< wumpus>
looks like we have a causality issue here, that message from provoostenator is visible here *just before* fanquake joining
< * provoostenator>
hides time machine
< wumpus>
leaky wormholes again
< fanquake>
wumpus :o
< fanquake>
wumpus I've retested 13823 now, thanks for pointing out the quoting issue.
< ken2812221_>
wumpus: I think #13426 can be postponed to 0.18, I don't think this can be merged in a week. I'll open an issue to track this and seperate to several PR for easier review
< MarcoFalke>
provoostenator: It builds on a single cpu on gce, heh. So maybe 24hours to build master and the commit for all targets ;)
< ken2812221>
Also, we'll 6+months to test if there is any side effect after these changes.
< MarcoFalke>
ken2812221: Yeah, was going to ask about the state of this.
< MarcoFalke>
Imo we should at least fix the crash
< MarcoFalke>
the crash on Windows for non-ascii wallet file name
< wumpus>
fanquake: cool, thanks for retesting
< wumpus>
ken2812221: ok!
< fanquake>
MarcoFalke which PR/changeset do you have in mind for that?
< MarcoFalke>
Idk what is causing the issue on windows
< wumpus>
ken2812221: changed milestone--thanks, I sort-of expected this, it was kind of a large and reasonably risky change to merge so last minute, better to do it as one of the first things for 0.18 probably
< fanquake>
Ah right, we're talking about #13754 here?
< MarcoFalke>
It reports a fatal error and then hits an assertion
< ken2812221>
MarcoFalke: That happen on -datadir for a really long time.
< MarcoFalke>
oh
< MarcoFalke>
Not a regression then
< fanquake>
Ok. I'm not entirely sure how we can solve that in not very intrusive, right before 0.17.0 kind of way.
< fanquake>
13426 currently has changes to bdb and leveldb as well
< provoostenator>
Afaik it used to throw a clear error and now it just crashes, but it's only a problem if you use character incompatible with your system language.
< MarcoFalke>
So not really a common issue to hit
< MarcoFalke>
Still, would be nice to bisect that
< MarcoFalke>
Will probably do that
< provoostenator>
Have fun, only takes 24 hours per build, right? :-)
< MarcoFalke>
heh, I hope I can steal them from jonasschnelli nightly server
< fanquake>
If someone wants to do some quick review, there is currently 13852 & 13852 for 0.16, both fairly simple backports
< provoostenator>
fanquake duplicate number, what's the second one? I'll take a look.
< fanquake>
provoostenator Sorry, 13796
< fanquake>
Another trivial one in 13853, not entirely sure how the versions got out of sync.
< provoostenator>
MarcoFalke: maybe it's useful to have a "needs windows build" tag? That's the only OS where you can't avoid cross-compilation pain.
< provoostenator>
fanquake: you're sure you want to bump QT to 5.9.6 in backports?
< wumpus>
let's not get too specific with labels
< provoostenator>
wumpus: it's just that the only reason I sometimes ask for Gitian builds is that I want to test Windows binaries.
< provoostenator>
Maybe others have other reasons.
< wumpus>
provoostenator: I understand, it's just that I think labels are best for keeping track of rough categories, this seems oddly specific :)
< provoostenator>
Oh wait, that one isn't a backport, nvm.
< provoostenator>
Maybe a Github bot listening for comments "Windows plz" is better than tags?
< wumpus>
I... think we should simply make sure enough CPU capacity is available for the build bot and build everything possible
< wumpus>
if cloud/server costs are a problem I'm sure we can find some solution
< fanquake>
Can run all the tests, fuzzers, linters, benchmarks, gitian builds, test sync times..
< wumpus>
yes, including openbsd and freebsd *ducks*
< ken2812221>
In my opinion, how about uploading travis build binaries to a server?
< ken2812221>
It would cost less money than own build.
< wumpus>
the problem, last time we looked at that option, had to do with credentials; as well as well as with potential malware, especially if PRs automatically upload binaries
< wumpus>
this is manually triggered by maintainers, so less risky
< ken2812221>
We can print the binary hash at the end and upload them to a close server. If someone want to get the binary, they can ask maintainer to get it. The downside is that we should trust travis-ci.
< luke-jr>
I don't really have a good solution for it
< wumpus>
#topic CXXFLAGS stuff
< gmaxwell>
Whats the issue?
< luke-jr>
gmaxwell: autotools forces user CXXFLAGS after our own; so when the user builds with -mno-avx2, the build simply fails
< provoostenator>
#13789
< gribble>
https://github.com/bitcoin/bitcoin/issues/13789 | crypto/sha256: Use pragmas to enforce necessary intrinsics for GCC and Clang by luke-jr · Pull Request #13789 · bitcoin/bitcoin · GitHub
< cfields>
eh?
< cfields>
it doesn't force it, we do that ourselves
< gmaxwell>
can someone please drop the registed users +q for now? sdaftuar is muted.
< luke-jr>
ie, autotools calls the compiler with our -mavx2 *followed by* -mno-avx2
< wumpus>
well apparaently there's a problem where the build system passes the wrong flags, and luke-jr's PR works around it with pragmas
< gmaxwell>
or at least voice sdaftuar
< wumpus>
that seems wrong to me
< cfields>
the intention is for any user-passed flags to be able to override the ones we add. If that's not the case, it's a bug.
< luke-jr>
cfields: that's the problem in this case
< luke-jr>
we can't let the user override -mavx2 here
< sdaftuar>
hi
< cfields>
luke-jr: you mean we shouldn't, or we currently don't?
< luke-jr>
cfields: autotools makes it impossible to override user CXXFLAGS, but for these files we must or we fail to compile
< gmaxwell>
luke-jr: why would the user ever pass -mno-avx2 in the first place? We shouldn't be using -mavx except on special files that need it to compile.
< cfields>
oh, I see what you mean
< luke-jr>
gmaxwell: to avoid AVX2 instructions
< cfields>
luke-jr: eh?
< luke-jr>
gmaxwell: it's those special files that fail to compile
< sipa>
hi!
< cfields>
luke-jr: fail how? do we just need to check for more intrinsics?
< provoostenator>
(I guess it was crickets and the muffled voice of sdaftuar in the dinstance)
< gmaxwell>
luke-jr: If the user wants to avoid executing avx2 instructions they need do nothing. They don't have to pass any special options.
< gmaxwell>
cfields: he is saying that if he builds with CXXFLAGS=-mno-avx2 the compile fails.
< luke-jr>
gmaxwell: AVX2 is enabled by default with some -march options
< cfields>
gmaxwell: I assume the issue is some failures to compile because of a busted compiler, so there's a desire to be able to avoid them entirely.
< gmaxwell>
luke-jr: why would anyone -march=<thing with avx2> then -mno-avx2? that just seems busted.
< cfields>
ooooooh
< wumpus>
it looks like a really contrives scenario to me
< luke-jr>
gmaxwell: I'm not sure why laurentb is doing it, but there's no reason they shouldn't be able to
< gmaxwell>
in any case, why not detect the -mno-avx2 and then don't even compile the file?
< wumpus>
not worth it polluting the code with all kinds of compiler specific pragmas at least
< cfields>
ok, there are plenty of ways to solve that. I think we can just discuss on the github issue?
< luke-jr>
gmaxwell: autotools says we're supposed to allow changing CXXFLAGS after configure
< wumpus>
agree with cfields , there's no hurry with this
< luke-jr>
make CXXFLAGS=…
< luke-jr>
okay
< gmaxwell>
in any case -march=<hardware that has avx2> -mno-avx2 sounds like a misguided thing that we shouldn't take a lot of complexity to support, unless someone knows otherwise.
< cfields>
gmaxwell: we turn off all flags except the ones we're testing when doing the autoconf checks so that they don't cause unrelated errors.
< gmaxwell>
Esp because there is -mtune
< luke-jr>
cfields: no we don't
< MarcoFalke>
Unassigned the issues from the 0.17 milestone for now
< cfields>
luke-jr: ones that we add, we do
< luke-jr>
MarcoFalke: it's a must-fix for 0.17
< wumpus>
no, it's not a must-fix for 0.17
< gmaxwell>
I don't see how this is a must fix.
< wumpus>
agree fully with MarcoFalke
< luke-jr>
broken build system..
< wumpus>
any other topics?
< gmaxwell>
"User sets weird options which seem to make no sense as we know, and then something that arguably should work but fails" is not really blocker material.
< meshcollider>
Lol
< provoostenator>
Windows
< provoostenator>
(topic)
< MarcoFalke>
I left the pulls for review in the 0.17 milestone, so if reviewers like them, they can still be merged
< luke-jr>
provoostenator: what about Windows? :p
< provoostenator>
As in: do we want to fix the Windows unicode stuff, given that there's still two weeks?
< wumpus>
right, it's annoying for the specific user, but if you have really specific needs like that you can patch around it
< MarcoFalke>
Let's do the unicode stuff for 0.18
< wumpus>
#topic Windows (provoostenator)
< provoostenator>
I think the opinion in the ticket was no.
< MarcoFalke>
It would require a leveldb bump and major changes
< gmaxwell>
wumpus: we should find out what the user is doing, might just be some greater confusion... like they want to benchmark each way or something.
< MarcoFalke>
Not sure if we can review and test that in such a short time frame
< provoostenator>
Ok, and we're not getting tons of reports about this either?
< jonasschnelli>
Is the unicode stuff just about filenames (wallet)?
< wumpus>
gmaxwell: right!
< MarcoFalke>
I am looking into restoring the proper warning for -wallet=non-ascii, but that should be all for 0.17
< MarcoFalke>
jonasschnelli: Also datadir
< sipa>
only half here, but feel free to ping me
< provoostenator>
I think it's mostly filename yes, but also labels.
< jonasschnelli>
Yeah. We should fix that. But this seems to be open since forever and I don't see a pressing need for 0.17
< provoostenator>
But afaik I know it works if your system locale is set "correctly".
< MarcoFalke>
jonasschnelli: Indeed, anything that is not a regression should go into 0.18
< cfields>
provoostenator: due to the nature of the bug, I think many of the people who would be reporting it may not speak english. So the significance may be a little under-represented.
< cfields>
s/bug/issue/
< gmaxwell>
I was trying to say what cfields just said.
< meshcollider>
Good point
< jonasschnelli>
Yes. Good point.
< luke-jr>
provoostenator: wait, labels are broken?
< gmaxwell>
We shouldn't take few reports to mean few issues... but it the change is invasive and not ready, and the issue isn't new...
< cfields>
gmaxwell: agreed. -1 from me as well. Just wanted to throw that out there.
< provoostenator>
luke-jr: not sure, try with an english system locale and then adding Chinese labels, I can't remember if that works.
< luke-jr>
provoostenator: last I checked, we had functional tests for non-English labels :/
< provoostenator>
But with a Chinese system locale it does work afaik, hence it doesn't seem super urgent.
< wumpus>
I think it's a serious issue
< MarcoFalke>
We can backport to 0.17.1, if it qualifies as bug fix
< provoostenator>
Functional tests that run on linux.
< wumpus>
but it's risky to do this last minute
< wumpus>
and it's not a regression
< gmaxwell>
MarcoFalke: +1
< wumpus>
MarcoFalke: agree
< provoostenator>
Maybe just merge it into master after the 0.17 split so it gets testing quickly. Then we could always backport it if there's strong demand?
< gmaxwell>
it's a bug, maybe one not worth backporting depending on how tidy the fix is.
< wumpus>
it's unfortunate that this requires such invasive changes for windows
< wumpus>
specific changes not required for other OSes
< ken2812221>
This is really hard to fix.
< wumpus>
which means most of use cannot test it usefully
< luke-jr>
can it be reproduced in WINE?
< provoostenator>
luke-jr: I haven't tried, I just use a virtual box with Windows 10
< MarcoFalke>
I don't think we should fall back to WINE for testing that issue
< provoostenator>
Someone recently offered to run and maintain Windows integration builds
< meshcollider>
It doesn't sound like WINE would have the same issue tbh
< gmaxwell>
Health professions recommend against trying to use wine to solve your problems.
< cfields>
careful we don't spiral to homebrew...
< MarcoFalke>
This really needs to be tested on native Windows (Not against testing it in wine additionally, though)
< MarcoFalke>
I could try once more and then write down the steps I did, heh
< cfields>
ken2812221: huh, we could potentially add it and upstream to mingw, then.
< wumpus>
why does utf-8 need a special fstream
< wumpus>
I'm confused
< wumpus>
why does this have to be so messed up
< ken2812221>
wumpus: just filename
< ken2812221>
It's fine to read the stream
< wumpus>
can't we just drop windows support
< * wumpus>
ducks
< meshcollider>
Lol
< provoostenator>
Maybe run it inside at little Linux VM? Can't be worse than Electron :-)
< wumpus>
yesss
< wumpus>
windows 10 already includes ubuntu right
< midnightmagic>
it's messed up because windows can still run old windows crap and they have to support old api forever, including all the crappy api they built when the company was on the rocks before they discovered scm
< wumpus>
let's dump the win32 garbage and use that...
< gmaxwell>
lol. really with the proposed builder stuff, would building a whole linux system really be worse? :P
< cfields>
wumpus: Doesn't a brand new win10 update add a bunch of compat stuff that would avoid these issues? I don't think it'd be crazy to consider dropping support for anything less than that reasonably soon.
< wumpus>
cfields: it does! that's true
< provoostenator>
Not by default, but you can install it. I once did the inception thing: Gitian builder VM inside Ubuntu inside Windows inside Virtual Box on Mac. It crashed.
< gmaxwell>
cfields: I dont know the details, but my understanding is that a lot of people don't want to run windows 10 due to integrated adware or something?
< provoostenator>
gmaxwell: the latest update, at least for me, even forced me to allow analytics data
< cfields>
whoa
< meshcollider>
Yeah it's much more invasive
< provoostenator>
They're definately going for the get hacked by random people on the internet or get spied on by us sales pitch.
< cfields>
gmaxwell: I suppose those people would be used to running outdated versions of things, then :\
< provoostenator>
(or both)
< wumpus>
ugh
< wumpus>
then again, this is not a regression
< wumpus>
what works on windows, works.
< gmaxwell>
thats a possibility too, and better than dropping support for windows...
< wumpus>
the question is whether we should change 10000 lines just to accomodate unicode in it
< gmaxwell>
It seems really surprising that we'd need to change more than some wrapper functions.
< luke-jr>
^
< wumpus>
yes it seems a design error in the first place if this cannot be wrapped somehow
< cfields>
agreed. And that's a reasonable thing to fix, imo. IIRC it also means we have trouble adding filesystem sandboxing.
< wumpus>
I do think ken2812221's PR is fairly ok in that regard
< wumpus>
yes the reason I did the fs:: stuff is to allow for filesystem sandboxing
< cfields>
wumpus: more is required though, right? I assume something along the lines of ken2812221's PR would help?
< wumpus>
cfields: I had it down to only changes in fs.h and adding a fs.cpp, afaik
< cfields>
ah, ok. nm then.
< wumpus>
but maybe more is needed now that the code evolved further...
< gmaxwell>
I hope we can find a set of changes that a reasonable independant of windows, so that the windows fix is just a couple files changed.
< wumpus>
I think my main drawback in ken2812221 's PR is that it changes .string to stringu8 or something, which seems unnecessary, all strings should be utf-8
< wumpus>
if you make sure the wrapper does that you don't need to change it everywhere in the code
< ken2812221>
wunpus: That's how boost's path work. It need to pass a utf8 converter.
< wumpus>
ken2812221: yes...
< wumpus>
but our own wrapper could do that automatically
< ken2812221>
Actually I have thought that before, but it need to patch boost.
< luke-jr>
didn't we want to get rid of boost anyway?
< wumpus>
or wrap boost
< luke-jr>
maybe this is a good opportunity
< wumpus>
we'll do so, in time
< wumpus>
yes
< wumpus>
any other topics?
< gmaxwell>
Topic suggestion: Leveldb FD usage on x86_64
< wumpus>
#topic leveldb FD usage on x86_64 (gmaxwell)
< gmaxwell>
There was a recent report from a user hitting the select limit on his x86_64 linux host. Inspection with lsof shows that leveldb is using a lot of FDs on nodes where we expected it to be mostly using mmap. Apparently leveldb has a number of mmaps limit, as far as I know there isn't any reason we shouldn't increase it.
< gmaxwell>
(seperately we should move to using poll ... but increasing the mmap limit should be a ~1 line change, unless someone knows a reason to not do so)
< wumpus>
I think this limit was increased recently
< wumpus>
specifically for 64-bit OSes, as it was deemed to be no problem
< wumpus>
of course, I'm not surprised it turns out to be it is... :/
< wumpus>
the reason to increase it was something with performance
< wumpus>
"utACK ccedbaf - with the comment that I think relying on undocumented behavior of leveldb is risky. "
< wumpus>
yeah, of course this is going to bite us in the ass
< wumpus>
so revert?
< cfields>
upstream has updated a bunch of stuff lately (namely moving to c++11 by default and dropping the need for lots of platform-dependent code). Has this been addressed as well, by any chance?
< gmaxwell>
wumpus: hm so reading this it sounds to me that there is a seperate limit of 1000 mmaps. And running into that limit is what is causing more FDs to be used than we expected.
< gmaxwell>
wumpus: so it might be possible to not revert that patch, but instead increase the maximum mmaps.
< cfields>
(that bump is going to be painful, btw)
< wumpus>
gmaxwell: I like switching to anything else than select() though!
< wumpus>
we've been stuck with select with no good reason
< gmaxwell>
wumpus: yes, I think we should do that too!
< sipa>
gmaxwell: that would explain why i saw exactly 999 mmaps
< wumpus>
my reason used to be, well, we're going to switch to using libevent for P2P any time now!
< gmaxwell>
BlueMatt: and phantomcircuit: both previously had private patches to change to poll.
< wumpus>
but that's been years
< wumpus>
let's just do it
< sipa>
wumpus: any time now!
< wumpus>
change to poll() for unix-ish OSes
< gmaxwell>
sipa: my comments above about the map limit are based on ossifrage (reporter's) work, and some of the comments in that PR above.
< cfields>
wumpus: sorry :(
< gmaxwell>
In any case, I think there are two issues: (1) switch to poll (duh but can we do it for 0.17) and (2) leveldb should probably be allowed more maps unless we know some reason to not do that.
< wumpus>
cfields: nooOO sorry I didn't mean it as an attack on you, I could have done it, many other people could have done it, but we didn't
< gmaxwell>
and maybe (3) potentially revert that PR; but given my understanding, I don't think we need to.
< gmaxwell>
Leveldb being map limited is probably going to end up bad for performance even if we no longer had the FD issues.
< luke-jr>
man mmap doesn't talk about a specific mmap limit
< wumpus>
there is no mmap limit afaik
< gmaxwell>
^
< gmaxwell>
Thats where my concern about increasing leveldb's mmap usage comes from-- I dont understand why the limit is there.
< cfields>
there's definitely something that limits it. Some unrelated sysctl ?
< cfields>
I know i've seen it documented somewhere.
< gmaxwell>
Unless they're trying to reduce TLB thrashing or something.
< wumpus>
cfields: for 0.18 we should definitely update leveldb though
< ossifrage>
I bumped my mmap file limit up to 4k, currently 1180 ldb files are mapped and it isn't using any fds for ldb files
< gmaxwell>
https://github.com/bitcoin/bitcoin/pull/12495#issuecomment-367815005 < eklitzke suggests increasing leveldb's mmap limit here. ossifrage also apparently did that and reported it fixed his problems, though his specific problems are likely better fixed by switching to poll.
< wumpus>
for 0.17 I'd propose to keep it like this, not do anything wild...
<@sdaftuar>
cfields: if i read correctly, the internet says sysctl vm.max_map_count
< gmaxwell>
I thought it was probably too late to switch 0.17 to poll, but increasing the leveldb map count seems like an easy mitigation that probably improves performance.
<@sdaftuar>
the comment there makes it sound like the 1000 mmap limit is some kind of memory limiter, but i don't know if i'm understanding correctly
< gmaxwell>
yep, seems like it.
< luke-jr>
ossifrage: I don't see that in the code. It looks like it just fails completely
< luke-jr>
s = IOError(fname, errno);
< ossifrage>
luke-jr, I was running into the 1000 map limit and then it started using a ton of FDs
< luke-jr>
ossifrage: I mean hitting the OS limit
< luke-jr>
ie, set your OS limit to 900
< gmaxwell>
I wonder if in the issue sdaftuar links the person is hitting the OS limit.
< ossifrage>
(ah, I thought you where talking about the limiter limit, duh)
< gmaxwell>
So leveldb added a maximum and set it to some random value.
< gmaxwell>
sdaftuar: thanks for finding that, though I wish it were enlightening. The user reports having enough memory. Google replies 'we fixed your running out of memory with an arbritary limit'...
< luke-jr>
seems like they should have at least made it so mmap failure falls back to using fds :/
< phantomcircuit>
gmaxwell, that does seem to be explicitly saying it's to limit the mmap memory usage to 2000MB
< luke-jr>
so was this a 32-bit problem only? I thought mmaps were only used on 64-bit?
< gmaxwell>
So I think maybe the limit was put in for 32 bit hosts... and then it was replied to someone who was really reporting another problem.
< gmaxwell>
luke-jr: we don't use mmaps on 32-bit hosts, but leveldb can...
< gmaxwell>
(not our leveldb)
< wumpus>
on 64 bit the limit is much higher
< gmaxwell>
gah, that was unclear lemme try. We don't use mmap on 32bit with leveldb, but that is a result of our configuration, since we already manage to use all the address space ourselves.
< gmaxwell>
wumpus: the leveldb mmap limit is also 1000 on 64-bit hosts.
< luke-jr>
so sounds like we should just set the mmap limit to infinite on 64-bit, and modify the mmap fail code to fallback?
< cfields>
so it'd disable mmap for x32 (not x86) as well, right?
< cfields>
(er, x86 too. but also x32 :)
< gmaxwell>
luke-jr: well not infinte, but something reasonably under 65530.
< luke-jr>
I guess we don't want to exhaust it for other apps
< luke-jr>
60k / number of bitcoinds launched by functional tests? :P
< gmaxwell>
setting it to just 4096 would put it well above our current usage. ... and by the time we hit that, we'll hopefully be using poll and it'll just be a performance consideration.
< gmaxwell>
ossifrage reports he's saying about 1180 maps.
< ossifrage>
gmaxwell, after 1.5 days of uptime
< ossifrage>
I started to see the socket problem after 20-30 days of uptime
< gmaxwell>
well how many ldb files are there in total?
< ossifrage>
from my last lsof, 992 from chainstate, 146 from txindex (currently). I have 1763 txindex ldb files and 1354 chainstate ldb files
< gmaxwell>
k, so 4096 would allow you to have all your ldb files mapped.
< wumpus>
leveldb's mmap limit doesn't correspond to any os-level mmap limit
< ossifrage>
Yeah, is that limit per database or process wide?
< ossifrage>
(the leveldb limiter limit)
< wumpus>
per database
< gmaxwell>
wumpus: yea, I think the history is that leveldb gained it due to 32bit. 2mb * 1000 = 2GB sounds a lot like someone trying to avoid VM exhaustion on 32bit.
< gmaxwell>
and they were only thinking about one database and ... yadda yadda.