Reduce per-field heap usage for indexed fields [LUCENE-6199]

[asfimport]

Lucene uses a non-trivial baseline bytes of heap for each indexed
field, and I know it's abusive for an app to create 100K indexed
fields but I still think we can and should make some effort to reduce
heap usage per unique field?

E.g. in block tree we store 3 BytesRefs per field, when 3 byte[]s
would do...

---

Migrated from LUCENE-6199 by Michael McCandless (@mikemccand), 1 vote, updated May 09 2016
Attachments: LUCENE-6199.patch (versions: 2)
Sub-tasks:

• Fix readStringStringMap api [LUCENE-6317] #7378
• re-use immutable maps across fieldinfos when they are the same [LUCENE-6318] #7379
• improve perf and memory of FieldInfos.fieldInfo(int) [LUCENE-6325] #7386

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Initial patch w/ nocommits...

This gives a 2.4x reduction (137 MB to 56 MB) in heap usage in a
simple test that creates 100K indexed fields in a single-segment
index.

I fixed Lucene50FISReader to share a single attributes map if it
notices that multiple fields have exactly the same attributes (the map
is read-only). It would be nice if we could fix this higher up,
e.g. fix PerFieldXXXFormat to not store its attributes if the format
is "the default" somehow.

It also moves some FST fields out to the builder, and adds a new
FST BytesReader impl for when it's a single byte[] page.

Separately, CodecReader.ramBytesUsed is missing some per-field heap:
it reports only 18.8 MB (out of 56 MB) with the patch ... I put some
nocommits for the ones I could find. I'll fix this in the next
iteration.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I fixed Lucene50FISReader to share a single attributes map if it
notices that multiple fields have exactly the same attributes (the map
is read-only). It would be nice if we could fix this higher up,
e.g. fix PerFieldXXXFormat to not store its attributes if the format
is "the default" somehow.

I don't think we should be doing this. This is letting the abuse case make our code (for field metadata, which is fucking important stuff!) more complicated.

-1: let them buy more ram.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Separately, CodecReader.ramBytesUsed is missing some per-field heap:
it reports only 18.8 MB (out of 56 MB) with the patch ... I put some
nocommits for the ones I could find. I'll fix this in the next
iteration.

Its not missing, it just doesnt report trivial things. This only impacts abuse cases. Please do not make Fieldinfos Accountable, etc. Its high time lucene stopped letting abuse cases screw over the average case.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Rob are you only vetoing the changes to Lucene50FIS? I really don't think that's so much added complexity, but if you are vetoing it, I'll revert that part.

And I'll just remove the nocommits about not accounting for RAM used by FieldInfos.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

All i want is a reasonable tradeoff. You know, if the code can be rearranged so this stuff is clear and isn't causing bugs for non-abuse cases, then I think its ok. But the current stuff seems to go far too far?

I don't fully get the blocktree changes without spending more time to understand if the right risk/reward tradeoffs (where reward is an abuse case) are being made, i mean maybe its fine, but could there be sneaky reuse bugs? Also why did we lose node/arc counts in stats? Was this on accident?

The FISReader attributes caching stuff seems extraordinarily risky. Are you sure it is really ok to suddenly become null where it was not before? Why not propose caching the attributes in SIReader too for abuse cases where people have too many segments?

i think adding Accountable to FieldInfos will ultimately be very invasive no matter how you do it? And in most cases, this is pretty ridiculous right? Shouldnt we instead think about non-abuse cases like adding LiveDocs to this ram computation before looking at making FieldInfos more complex?

Honestly, if the right tests are in place, i think I would get a lot less upset about it. But i don't like the idea of introducing bugs, that hurt real use cases, caused by complexity of optimizing abuse cases. Do you agree or disagree these changes are really scary?

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

All i want is a reasonable tradeoff.

Well, that is what I tried for in the current patch ... I think there are further things we could explore (e.g. packing N longs, which are often small) that I didn't do because that seemed too much.

Also why did we lose node/arc counts in stats? Was this on accident?

This wasn't by accident: you have access to these from the FST.Builder, and you can save them away if you really need them later (nothing does in Lucene).

i think adding Accountable to FieldInfos will ultimately be very invasive no matter how you do it?

OK I won't add Accountable to FIS...

Honestly, if the right tests are in place, i think I would get a lot less upset about it. But i don't like the idea of introducing bugs, that hurt real use cases, caused by complexity of optimizing abuse cases. Do you agree or disagree these changes are really scary?

I agree FIS.getAttributes should not be null, and I'll add a test case for that. I'm happy to add further tests, what do you have in mind? But net/net, no, I don't think these changes are scary: they look low risk to me, and they give an enormous reduction on per-indexd-field RAM used.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

But net/net, no, I don't think these changes are scary: they look low risk to me, and they give an enormous reduction on per-indexd-field RAM used.

First of all, no: the changes do not give even a noticeable reduction of RAM for 99.9% of users. Thats my number one problem.

Second, I am disappointed that you see this as low-risk. Perhaps the biggest risk of all is doing this with a shitty risk-reward. The reward is nothing, since it only impacts abuse cases.

But ill stay out of the way. This kinda stuff is why the codebase gets so damn complicated, because, without any reservations at all, we'd greatly overcomplicate very central core classes to lucene for use cases that flat out do not exist.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Maybe as a compromise, you can try to make a much smaller patch here that makes some progress on the abuse case you want to help.

But the current patch is a big problem, it adds WHOLE CLASSES to lucene and tons of crazy code.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

adds WHOLE CLASSES to lucene and tons of crazy code.

OK ... I think I have an alternative approach that doesn't need the new classes for < 1 GB sized FSTs (the common case by far). I'll try it.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Maybe the fieldinfos changes can be split out to a separate patch as well. this should be tiny and we can probably contain it and feel safe about it.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

New patch with a simpler approach:

• I took a different approach for < 1 GB FSTs, and removed the two
  added classes.

• Removed all nocommits about per-field RAM accounting being wrong.

• Added a test case to BaseFIFormatTestCase to assert that a field's
  attributes are never null; this failed in my last patch so I fixed
  it to pass an empty map instead.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Maybe the fieldinfos changes can be split out to a separate patch as well.

This change is really small and does make a big difference on per-field RAM usage.

But if you are really so against it, I can just remove it entirely?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

This change is really small and does make a big difference on per-field RAM usage.

I don't think it does? If i have an index with say, 10 fields. How much will this patch save me?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

New patch with a simpler approach:

Its still a 51KB patch. This is too much for an abuse case. It needs to be less invasive.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

btw: I think you still missed my entire point. This is a big scary patch, to help only an abuse case.

It brings incredible risk to all lucene users, just to help a few that licked lead paint growing up.

So if we can break it down into digestible chunks, then I feel we can make steps to simplify the logic, carefully review changes, and improve tests to help reduce that risk. One digestible chunk at a time.

But if the only way this is gonna go in, is a huge scary patch that rips across the codebase changing codec code, then yeah, we shouldnt do it.