I18NBundle.createBundle takes unreasonable time on Android

[titovmaxim]

The project has two simple translation files: one base (bundle.properties) and one for the language of choose (bundle_ru.properties). So the chain is very short - just two files. The files are simple and small (about 5 Kb).

I18NBundle.createBundle works ok on Desktop. However on Android in takes unreasonable time to load. On Galaxy S III it takes 0.8 sec. Apparently there is some bug or a problem. Could be related to very-very slow check is a directory exists on android internal files.

This make I18NBundle close to unusable on Android. Changing from I18NBundle to simple json load of two similar files solves the problem in no time.

[davebaol]

Well, unless you create one bundle per second I wouldn't say that 0.8 seconds make it close to unusable. :)
Anyways, can you profile it?

[titovmaxim]

Well, you are right. But during the game start of lot of things are usually loaded. Another second for nothing is a pity :) The simple hand-made solution with jsons do the same thing and consumes no time at all.

Not sure how to profile third-party libraries on Android. Usually I just install own timers in a valuable places of my code. But what to do with a library? Can you give an idea?

[badlogic]

DDMS allows you to profile any code in your Android app.
On Sep 17, 2014 9:46 AM, "titovmaxim" notifications@github.com wrote:

Well, you are right. But during the game start of lot of things are
usually loaded. Another second for nothing is a pity :) The simple
hand-made solution with jsons do the same thing and consumes no time at all.

Not sure how to profile third-party libraries on Android. Usually I just
install own timers in a valuable places of my code. But what to do with a
library? Can you give an idea?

—
Reply to this email directly or view it on GitHub
#2342 (comment).

[titovmaxim]

Great. Thanks. Profile shows that all time is consumed by AnfroidFileHandle.exists(). Like I assumed before :) Therefore on Desktop you can not notice it.

[titovmaxim]

P.S. Because of this I use internally this function:

public static boolean fastFileExistsCheck(FileHandle fh)

{

try {

fh.read().close();

return true;

}

catch (Exception e) {

}

return false;

}

[davebaol]

Hmmm.... It's a known issue.
From http://libgdx.badlogicgames.com/nightlies/docs/api/com/badlogic/gdx/files/FileHandle.html#exists--

Note that this can be very slow for internal files on Android!

[titovmaxim]

Yes. And I guess this line about "very slow" in comments was introduced after my previous issue report :)

That time I have even suggested to change the default implementation. My idea was to restrict .exists() only to files, excluding directories (the reason why it is so slow). Something like my function above. And make a separate function which includes directories. But the solution was to add a line in comments :)

[davebaol]

Yep more than 1 year ago https://code.google.com/p/libgdx/issues/detail?id=1655

[titovmaxim]

What if just to remove the check if the bundle exists and try to load it directly? Then catch the exception, in the case it does not exists... (I18Bundle line 319).

[davebaol]

With your workaround a directory would be silently skipped and you won't get an exception.
We might still consider this acceptable though.

@badlogic @NathanSweet @MobiDevelop
What's your thought?

[MobiDevelop]

Using exceptions for flow control doesn't seem "right" but I guess it wouldn't be the end of the world.

[davebaol]

Yeah, I meant if we should fix this in I18NBundle or in FileHandle (providing an additional method or what else)

[MobiDevelop]

I don't think we should change the behavior of the existing exists method. If the question is whether we should add an additional exists method which bypasses checking if a directory exists, I don't have a problem adding such a method.

[badlogic]

Additional exists is a bit meh, but the lesser of two evils...
On Sep 18, 2014 12:50 AM, "Justin Shapcott" notifications@github.com
wrote:

I don't think we should change the behavior of the existing exists
method. If the question is whether we should add an additional exists
method which bypasses checking if a directory exists, I don't have a
problem adding such a method.

—
Reply to this email directly or view it on GitHub
#2342 (comment).

[davebaol]

Then how about adding openQuietly to StreamUtils? Note that closeQuietly is already there. :)

    /** Creates a stream for reading the specified file as bytes and ignore all errors.
     * @return the input stream or {@code null} if the file handle represents a directory, doesn't exist, or could not be read. */
    public static InputStream openQuietly (FileHandle fh) {
        try {
            return fh.read();
        } catch (Throwable t) {
            return null;
        }
    }

[titovmaxim]

I think any of the solutions will fit the current topic. However we will have this problem again next time. People would continue to use .exists since the problem is not wide-known and no one expect performance jam from such a simple function.

At the same time I guess it is not good idea to change .exist behavior (despite I doubt that many people use it for directory check in the frame of libGDX).

Probably the solution could be just to add a function with the name like .existsFastFileCheck. Then the people at least would see it automatically in autocomplition when they start to type .exists.

[davebaol]

@titovmaxim
I'd agree about autocompletion.
Anyways, are you sure that the check is faster with read()?
Just noticed that it calls file().exists().

[titovmaxim]

I'm not 100% sure (in a trip right now), but there is a difference between file.exists and FileHandle.exists. First one is ok, second one is slow.

The second one is slow (if I remember correct, was a long time ago) just because if it fails with file.exists check, it tries to do FileHandle.class.getResource. (line 526 of FileHandle - fall through comment). It is done to assure correct check of directories. However in the comment of these function it is written that "On Android, a {@link FileType#Classpath} or {@link FileType#Internal} handle to a directory will always return false". So no reason for such slow check if should anyway return false. I'm a bit confused here.

Probably just remove this last check only for android and internal files? Then it should work fast and according to the comments of the function.

[davebaol]

@titovmaxim
Just to clarify, AndroidFileHandle.exist() overrides FileHandle.exist(). When the file is not found on the internal filesystem assets.list(fileName) is executed to check directories, which makes exists() slow on Android, see https://github.com/libgdx/libgdx/blob/master/backends/gdx-backend-android/src/com/badlogic/gdx/backends/android/AndroidFileHandle.java#L194
AndroidFileHandler.read() is very similar to exists() except for directory scan, which is not needed in this case. This explains why your workaround is faster on Android.

[MobiDevelop]

Yesterday, I experimented with an alternative approach to handling the exists check on Android. Instead of attempting to open the file and falling back to listing the directory, it reads the manifest from the apk and caches the paths. This has had quite promising results so far.

[davebaol]

Interesting approach.

[mikemee]

Leaving aside the generic Exists slowness, for the I18NBundle case that this bug targets, it is possible to use one of the workarounds inside its code? E.g. Instead of calling fileHandle.exists() here https://github.com/libgdx/libgdx/blob/master/gdx/src/com/badlogic/gdx/utils/I18NBundle.java#L319, how about one of the alternatives suggested above?

Will you accept a PR if I try it? Any preference on OpenQuietly vs fastFileExists above?

Side note: the exist check for i18n also causes problems for iOS: #2345, so I'm doubly motivated.

[MobiDevelop]

Just remove the existence check in I18NBundle and call it a day.

I am opposed to adding an openQuietly method and adding a special method to accommodate an implementation detail in a single backend for a single file type (Internal) does seem like overkill... I'd think it is better to fix the actual problem than to add hacks to get around it.

[davebaol]

ok adding the workaround in I18NBundle right now.