Hi @fmeum. Is the test being fixed currently?

On it. Still trying to find the least bad fix.

@fmeum Any update on this? We hope to get the 9.0.1 RC1 tomorrow

I talked about a fix with @lberki and can likely finish it tomorrow.

What's the expection RAM regression of this?

@lberki is running a benchmark

@iancha1992 let's defer this to a 9.0.2 release if it's the last blocker for 9.0.1 - the fix is too uncertain at this point

Uh, I can run a benchmark once there is code, but I'm not at the moment. What I did do is to check how many source artifacts there are in a large build (I can tell you the exact one in private) and the end result is ~500K. With 16 bytes per Java object that comes out to 8MB of wastage for the wrappers.

Reasonable, I think, but we do have a few builds dancing on the edge already so I didn't want to agree to it just willy-nilly.

@lberki This is functional now - could you run a benchmark?

I had to drop the wrapper optimization since lookup via a PathFragment would not match a wrapper. Alternatives would be to use a ConcurrentSkipListMap with a custom comparator to avoid the wrapper or to vendor in a ConcurrentHashMap implementation and make hashCode overridable. Neither of those sounds great to me.

I tested this on Bazel's own build and found ConcurrentSkipListMap to use the least memory. I switched to it now.

@lberki @pzembrod What's the status on this? We want to include this fix in our upcoming 9.0.2 RC1 on Wednesday.

I ran some benchmarks. The first one is encouraging: the memory impact for n=5 builds is ~7MB median, which is about half a standard deviation.

I kicked off some more benchmarks just in case, because it has happened in the past that my ad hoc benchmark said "OK" and it wasn't.

Next set of benchmarks is doubly encouraging: it was a big build without special focus on C++ and the memory hit is well within measurement error. So on the memory use side, we're good.

Let me know when I should actually review the code of this pull request!

@lberki It's ready now, thanks for all the testing!

@lberki Please let me know if you need help merging this. Thanks!

I drafted 2 changes on top of your code:

• Option 1 - Allow validateInclusions to handle case-insensitive file systems. fmeum/bazel#8 tries to ignore case / owner differences in the inclusion check
• Option 2 - Fix case-insensitive header discovery and validation. fmeum/bazel#9 rewrites part of header discovery to eliminate the possibility of returning headers in the wrong case. The cost is cache efficiency when case-insensitivity is needed, since we will require scanning the action inputs at all times.

I feel like if we allow having incorrect cases (the current impl), we should at least allow option 1.

Please review the changes @lberki