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

IndexInput.isLoaded seems to return false for mmap index inputs on Windows #14050

Closed
dweiss opened this issue Dec 9, 2024 · 16 comments · Fixed by #14053
Closed

IndexInput.isLoaded seems to return false for mmap index inputs on Windows #14050

dweiss opened this issue Dec 9, 2024 · 16 comments · Fixed by #14053
Labels

Comments

@dweiss
Copy link
Contributor

dweiss commented Dec 9, 2024

Description

There is a check added to BaseDirectoryTest case in #13998 which has been failing since that PR was merged (nearly all Windows builds failing on jenkins):

        var loaded = in.isLoaded();
        if (FilterDirectory.unwrap(dir) instanceof MMapDirectory
            // direct IO wraps MMap but does not support isLoaded
            && !(dir.getClass().getName().contains("DirectIO"))) {
          assertTrue(loaded.isPresent());
          assertTrue(loaded.get());
        } else {
          assertFalse(loaded.isPresent());
        }

The isLoaded optional has a value of false, even though it is a mmap directory. Seems like isLoaded in MemorySegment always returns false on Windows?

Version and environment details

No response

@dweiss dweiss added the type:bug label Dec 9, 2024
@dweiss
Copy link
Contributor Author

dweiss commented Dec 9, 2024

Here is an example build failure:
https://jenkins.thetaphi.de/job/Lucene-main-Windows/14859/

These only happen on Uwe's machine but can be reproduced on any Windows machine.

@dweiss
Copy link
Contributor Author

dweiss commented Dec 9, 2024

@dweiss
Copy link
Contributor Author

dweiss commented Dec 9, 2024

While the test can be fixed easily (bypass the check on Windows), I wonder if it's a good fix. In essence, this method lies on Windows... What are the side-effects of this always returning false?

@rmuir
Copy link
Member

rmuir commented Dec 9, 2024

The method returns Optional which means its not a boolean but a quadrillean or something instead, on windows we could return empty-optional or even null to say "don't know" ?

@dweiss
Copy link
Contributor Author

dweiss commented Dec 9, 2024

Yep, I think I understand the reason it returns Optional - a missing value would indicate "don't know", so it's a tri-state knob: yes, no, don't know. The problem is that on Windows it returns "no" and that logic in BaseDirectoryTest assumes that if it's a mmapdir, it should always be a hard "true".

@rmuir
Copy link
Member

rmuir commented Dec 9, 2024

yeah, i'm suggesting adding this to the code:

if (Constants.WINDOWS) {
  return DONT_KNOW;
}

@dweiss
Copy link
Contributor Author

dweiss commented Dec 9, 2024

This was my initial guess too. But if we hardcode it and then they fix the JDK with some magic Windows API that returns the right value? Should we try to detect if isLoaded returns true on Windows and return the detected value?

@rmuir
Copy link
Member

rmuir commented Dec 9, 2024

It seems the commented-out code with hardcoded false is also in the JDK master? I think we should at least report the bug since it is impacting us.

I am not sure it needs a magic windows API, just looks like maybe the wrong one (VirtualQuery) was attempted instead of e.g. QueryWorkingSetEx

@rmuir
Copy link
Member

rmuir commented Dec 9, 2024

the git blame for the commented-out code shows it is at least 17 years old. The QueryWorkingSetEx was not even around at that time.

@rmuir
Copy link
Member

rmuir commented Dec 9, 2024

At least not everywhere: Minimum supported server: Windows Server 2008. So maybe this one can be revisited in 2024 and fixed...

@dweiss
Copy link
Contributor Author

dweiss commented Dec 9, 2024

I've peeked at the flags returned by QueryWorkingSetEx - just wondering if it'd be possible to query it directly using ffi. :) But I can't tell which of these flags would correspond to the loaded/ swapped out state. Fun.

https://learn.microsoft.com/en-us/windows/win32/api/psapi/ns-psapi-psapi_working_set_ex_block

But I agree - reporting back to JDK is probably a saner idea, I'm sure they have better win32 api experts there.

@rmuir
Copy link
Member

rmuir commented Dec 9, 2024

I think it won't return flags at all in that case.

@dweiss
Copy link
Contributor Author

dweiss commented Dec 9, 2024

I don't have access to openjdk's Jira, maybe @ChrisHegarty or Uwe (or you?) can file an issue there?

@ChrisHegarty
Copy link
Contributor

ChrisHegarty commented Dec 9, 2024

Oops! Sorry, clearly I added this test and didn't noticed it fail on Windows. For Lucene, I'm perfectly fine with returning "don't know" for all Windows. Lemme look into why this is not working in the JDK. It would be good to ensure that there is an issue open for this in the OpenJDK JIRA, but I'm less inclined to spend much time trying to make it work in Lucene until it is fixed in the JDK.

@dweiss
Copy link
Contributor Author

dweiss commented Dec 10, 2024

Thanks, Chris. The only Windows tests running at the moment are on Uwe's vbox. Github's Windows testing is so slow as to be almost unusable. The results from Uwe's box are sent to this list -
https://lists.apache.org/[email protected]

@uschindler
Copy link
Contributor

Hi,
I would do it like the following on Windows:

  • if windows returns true, return Optional.of(Boolean.TRUE)
  • if windows returns false, return Optional.empty()
  • once the bug is fixed in some future JDK feature version, add a version.feature()>=... check to return the unfiltered variable.

Not sure if the test needs to be adapted.
Uwe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants