Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Equal default min and max heap settings #16334

Merged
merged 1 commit into from May 11, 2016

Conversation

Projects
None yet
7 participants
@jasontedor
Copy link
Member

commented Jan 31, 2016

Today we encourage users to set their minimum and maximum heap settings
equal to each other to prevent the heap from resizing. Yet, the default
heap settings do not start Elasticsearch in this fashion. This commit
addresses this discrepancy by setting the default min heap to '1g' and
the default max heap to the default min heap.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2016

@mikemccand @rmuir This is why the Elasticsearch trace that we were looking at a few days ago showed the committed heap not starting at 1g but instead started at 256m and stair-stepped up to 1g.

@dadoonet

This comment has been minimized.

Copy link
Member

commented Jan 31, 2016

++

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2016

Hmm, thinking about this more, I'm not sure we should do this?

Yes, we do make this recommendation, to maximize performance for e.g. production installations.

But when someone is just launching ES at defaults, playing with a node or three on their laptop, I think the current default is maybe better, such that java only asks the OS for a smallish up front chunk of RAM for each java process?

@bleskes

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

But when someone is just launching ES at defaults, playing with a node or three on their laptop, I think the current default is maybe better, such that java only asks the OS for a smallish up front chunk of RAM for each java process?

I'm not sure what the background of the PR is (it seems to be a follow up on another discussion), but +1 to what Mike said - our current defaults are geared towards downloading ES and just trying it out/dev mode. As such, we shouldn't just claim 1GB of memory..

@jasontedor jasontedor closed this Feb 1, 2016

@jasontedor jasontedor deleted the jasontedor:min-heap branch Feb 24, 2016

@jasontedor jasontedor restored the jasontedor:min-heap branch Apr 27, 2016

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2016

@clintongormley I'd like to reconsider this pull request since we so strongly encourage this to be the case and since #17728 we warn on startup if it is not the case (and even fail if bound to a non-loopback interface).

[2016-04-27 10:03:59,228][WARN ][bootstrap                ] [Songbird] initial heap size [268435456] not equal to maximum heap size [1073741824]; this can cause resize pauses and prevents mlockall from locking the entire heap

I think one possibility is to set the default min and max to 256m or 512m if the concern is that 1g is too much?

@jasontedor jasontedor reopened this Apr 27, 2016

@clintongormley

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

since #17728 we warn on startup if it is not the case (and even fail if bound to a non-loopback interface).

If we set min/max to the same value at startup, we lose the ability to detect that the heap hasn't been set correctly when moving into production. Given that we fail hard in production if not set, what is the benefit of making this change for the OOB case?

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

If we set min/max to the same value at startup, we lose the ability to detect that the heap hasn't been set correctly when moving into production.

That's a good point, but easily adjusted to. We could fail if non-equal or if the heap is small (< 1 GB)?

Given that we fail hard in production if not set, what is the benefit of making this change for the OOB case?

I continue to find it odd that we so strongly encourage the practice but do not ship with it configured as such.

@clintongormley

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

I continue to find it odd that we so strongly encourage the practice but do not ship with it configured as such.

We ship for a good OOB dev experience. We don't know if somebody is going to run one instance and try to fill it with data, or start 4 nodes on their laptop to check out clustering...

Personally I like the flexibility of the current min/max, but I may be missing something

@clintongormley

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

One thing is for sure, we expect people to set the heap to something other than whatever we default to.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

If we set min/max to the same value at startup, we lose the ability to detect that the heap hasn't been set correctly when moving into production.

@clintongormley Reflecting on this further, I see the point of the min/max size check differently. I see the point as ensuring that the min/max are the same, not that the values have been tuned from their defaults. On any reasonable production workload, the user is going to find out very quickly if they haven't tuned away from the defaults, especially if we tune down to 256m or 512m like I'm proposing.

We ship for a good OOB dev experience. We don't know if somebody is going to run one instance and try to fill it with data, or start 4 nodes on their laptop to check out clustering...

That's why I'm proposing to tune it down to 256m or 512m.

Personally I like the flexibility of the current min/max, but I may be missing something

I think I'm missing what the flexibility is? What I see is us repeatedly communicating to set min equal to max, and then not shipping as such.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

One thing is for sure, we expect people to set the heap to something other than whatever we default to.

Exactly. And if they don't and are doing anything serious, they will know very quickly (especially if we tune it down to satisfy running multiple nodes on a laptop).

@clintongormley

This comment has been minimized.

Copy link
Member

commented May 5, 2016

I've been thinking about this some more and am swinging around to the other side. With things as they are today, if a user needs to bind to anything other than localhost in their dev environment, then suddenly they have to specify a bunch of stuff: min master nodes, min/max heap size, etc...

We now have an escape hatch for this situation (#18088) but I am wondering if it would be better for us to ship with min=max heap as a default, perhaps 500M?

If we set min/max to the same value at startup, we lose the ability to detect that the heap hasn't been set correctly when moving into production.

That's a good point, but easily adjusted to. We could fail if non-equal or if the heap is small (< 1 GB)?

This would kinda defeat the reason that I've suggested for shipping with min=max...

Still undecided

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented May 5, 2016

We now have an escape hatch for this situation (#18088)

Note that that only covers "system" checks, not checks that the end-user definitely has complete control over.

but I am wondering if it would be better for us to ship with min=max heap as a default, perhaps 500M?

Yes. 😄

That's a good point, but easily adjusted to. We could fail if non-equal or if the heap is small (< 1 GB)?
This would kinda defeat the reason that I've suggested for shipping with min=max...

Yeah that's a good point; I implicitly backed away from this in a previous comment for different reasons.

Still undecided

That's fair. Thanks for still being open to the proposal. (Note that I haven't pushed a commit to make the min equal to the max equal to 512 MB, but will if we come to agreement.)

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented May 11, 2016

@clintongormley I've updated the PR as discussed.

Equal default min and max heap settings
Today we encourage users to set their minimum and maximum heap settings
equal to each other to prevent the heap from resizing. Yet, the default
heap settings do not start Elasticsearch in this fashion. This commit
addresses this discrepancy by setting the default min heap to '512m' and
the default max heap to the default min heap.

@clintongormley clintongormley added >enhancement and removed discuss labels May 11, 2016

@clintongormley

This comment has been minimized.

Copy link
Member

commented May 11, 2016

I've updated the PR as discussed.

Some background to the discussion. We currently ship with min != max, which means that we log a warning about bad settings out of the box. Also, binding to anything other than localhost immediately requires editing a bunch of things to make elasticsearch work, even if we're actually still in development mode. Shipping with min==max means we (a) don't log a warning by default and (b) there's one less thing to edit.

We've chosen 512m as it's enough space to hold a fair bit of data while not so much that you can't start a few nodes on your dev machine. If there isn't enough memory available, the node will fail at startup so the problem will be obvious.

LGTM

@jasontedor jasontedor removed the review label May 11, 2016

@jasontedor jasontedor merged commit e4edee5 into elastic:master May 11, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@jasontedor jasontedor deleted the jasontedor:min-heap branch May 11, 2016

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

is 512 MB really enough memory for ES to run? https://benchmarks.elastic.co/index.html

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented May 12, 2016

is 512 MB really enough memory for ES to run?

It's not enough for the benchmarks to run at their previous level of throughput, but I don't think that should dictate the defaults that we use here because these defaults are oriented towards the out-of-the-box experience, not production and not the benchmarks.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

I ask if its enough memory for ES to run when GC times increase 25x

The dataset being used there is tiny.

@clintongormley

This comment has been minimized.

Copy link
Member

commented May 12, 2016

@rmuir what's your take on setting min==max vs what we do today (min=250m, max=1g)

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

I think we should use as little RAM as possible: but in this case I am saying that something is wrong.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

and @clintongormley yes, min == max does not really matter to me at all. I don't agree with the mlockall and all that, I think everyone knows that. We should keep that stuff separate.

Here, I think we should look at that old GC chart. Something is wrong. Remember ES is a ton of java classes with a lot of dependencies. There are various RAM pigs swimming around in the code too. It may very well be that 512MB is simply not enough.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

It's not enough for the benchmarks to run at their previous level of throughput, but I don't think that should dictate the defaults that we use here because these defaults are oriented towards the out-of-the-box experience, not production and not the benchmarks.

Besides the fact that something is truly wrong, I dont agree with this. People are going to benchmark the out of box configuration no matter what we tell them.

But this statement also disagrees with the change you made. mlockall is a (bad) thing that we recommend for production, its a production change. so I don't think it should dictate the defaults, just like you said :)

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

Hi, I don't handle Logstash customers; but my real fulltext customers get the following advice:

  • Disable mlockall in really any case. Bad bad bad (sorry, like @rmuir I disagree here completely. mlockall is stuff only for very special small processes that need realtime features; but not monster processes with tens of gigabytes of heap)
  • Set swappiness to 1 (not 0 !!!)
  • Enable MMAP from the beginning (nuke the hybrid one) and use Docvalues for sorting or facetting.

Customer is happy, their infrastructure does not complain and the system is stable also when somebody tries some heavy query or starts some amok process on the machine. With swappiness=1 the system at least has an option to recover and not crush with kernel panic or OOM killer killing your SSH daemon.

Nothing more to say from my side!

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented May 12, 2016

People are going to benchmark the out of box configuration no matter what we tell them.

The benchmarks allocate around 335 GB in the TLABs and 40 GB out of of the TLABs during the indexing phase alone with 512m and 1g heaps. These heaps (whether it be 512m or 1g) are tiny for sustaining high throughput here (442.44 MB/s on the 512m heap and 1.15 GB/s on the 1g for the sustained allocation rate in the TLABs).

If I increase the heap size for the benchmarks to 2g, the allocations shift to even more into the TLABs at a sustained rate of 1.8 GB/s during indexing. And if I increase the heap size for the benchmarks to 4g, the allocation profile is basically unchanged from 2g (roughly the same amount in the TLABs and rate during indexing).

So my conclusion is that 512m and 1g are just too small to begin with for the benchmark workload.

Also, this issue has nothing to do with mlockall et al. so I think we should put that discussion aside for another time?

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

So my conclusion is that 512m and 1g are just too small to begin with for the benchmark workload

I index this particular dataset all the time with lucene, on ancient computers. I do this because its tiny and fast.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented May 12, 2016

I index this particular dataset all the time with lucene, on ancient computers. I do this because its tiny and fast.

That's not a valid comparison because there are no networking buffers and all of the other functionality of Elasticsearch that sits on top of Lucene. And more importantly, it does not negate what I've shown here. The benchmarks want to sustain 1.8 GB/s of allocation here1, and 512m and 1g heaps are not going to allow them to do that.

1: On my workstation with i7-5930K, 2133 MHz DDR 4, Samsung 950 Pro M.2 SSD.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

Its totally valid. The workload is trivial.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

By the way... It is not that i am against your change. It is just that defaults are obviously broken. Defaults need to support indexing a trivial dataset. I have indexed this one over the years with many many configurations... Including 256mb heap with solr. I will open a separate blocker issue about this, because it can be fixed in two ways. We can give more ram by default... Or we can use less of it. But the defaults are broken.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented May 13, 2016

It is just that defaults are obviously broken. Defaults need to support indexing a trivial dataset.

I do not disagree with this. That was the point of my work, to show that even 1g is too small for this dataset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.