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

Cache paths in Directories class #12572

Open
koppor opened this issue Feb 25, 2025 · 10 comments
Open

Cache paths in Directories class #12572

koppor opened this issue Feb 25, 2025 · 10 comments
Labels
dev: performance status: waiting-for-feedback The submitter or other users need to provide more information about the issue

Comments

@koppor
Copy link
Member

koppor commented Feb 25, 2025

JabRef is very slow (250ms) when checking for a dirctory path in org.jabref.logic.util.Directories.

Image

Task

  1. Create static variable AppDirsFactory.getInstance()
  2. Cache results of get_X_Directory(), so that the result is determined once and re-used at subsequent calls.

(This is a work around for harawata/appdirs#117)

@koppor koppor added dev: performance good first issue An issue intended for project-newcomers. Varies in difficulty. labels Feb 25, 2025
@github-project-automation github-project-automation bot moved this to Free to take in Good First Issues Feb 25, 2025
@lalit2506verma
Copy link

/assign-me

@github-actions github-actions bot added the 📍 Assigned Assigned by assign-issue-action (or manually assigned) label Feb 25, 2025
Copy link
Contributor

👋 Hey @lalit2506verma, thank you for your interest in this issue! 🎉

We're excited to have you on board. Start by exploring our Contributing guidelines, and don't forget to check out our workspace setup guidelines to get started smoothly.

In case you encounter failing tests during development, please check our developer FAQs!

Having any questions or issues? Feel free to ask here on GitHub. Need help setting up your local workspace? Join the conversation on JabRef's Gitter chat. And don't hesitate to open a (draft) pull request early on to show the direction it is heading towards. This way, you will receive valuable feedback.

Happy coding! 🚀

⏳ Please note, you will be automatically unassigned if the issue isn't closed within 45 days (by 11 April 2025). A maintainer can also add the "📌 Pinned"" label to prevent automatic unassignment.

@ThiloteE ThiloteE moved this from Free to take to Assigned in Good First Issues Feb 26, 2025
@lalit2506verma
Copy link

lalit2506verma commented Feb 26, 2025

Hello @koppor,

I want to confirm my understanding of the task. Should AppDirsFactory.getInstance() be cached so that it can be reused in the get_X_Directory() method? Please let me know if this aligns with the requirement.

Screenshot for reference

[EDIT]
Image

@ungerts
Copy link
Contributor

ungerts commented Mar 1, 2025

@koppor and I intend for the same instance returned by AppDirsFactory.getInstance() to be used for creating all directory paths. This instance should be cached, for example, using a singleton. Additionally, we should investigate where time is being lost.

My experiments indicate that the first call to Shell32Util.getKnownFolderPath(convertFolderIdToGuid(folderId)) in the ShellFolderResolver class can take up to 260 ms.

Image

Below is my experimental class:

package org.jabref.logic.util;

import java.io.File;
import java.nio.file.Path;

import net.harawata.appdirs.AppDirs;
import org.jabref.logic.ai.AiService;
import org.jabref.logic.os.OS;
import org.jabref.model.search.LinkedFilesConstants;

import net.harawata.appdirs.AppDirsFactory;

/**
 * This collects all directories based on AppDirs.
 * OS-dependent directories are handled in the NativeDesktop class.
 * See e.g. {@link org.jabref.gui.desktop.os.NativeDesktop#getApplicationDirectory()}
 */
public class Directories {

    private Directories() {
        throw new IllegalStateException("Utility class must not be instantiated");
    }

    /**
     * Returns the path to the system's user directory.
     *
     * @return the path
     */
    public static Path getUserDirectory() {
        return Path.of(System.getProperty("user.home"));
    }

    public static Path getLogDirectory(Version version) {
        return AppDirsEnum.INSTANCE.getLogDirectory(version);
    }

    public static Path getBackupDirectory() {
        return AppDirsEnum.INSTANCE.getBackupDirectory();
    }

    public static Path getFulltextIndexBaseDirectory() {
        return AppDirsEnum.INSTANCE.getFulltextIndexBaseDirectory();
    }

    public static Path getAiFilesDirectory() {
        return AppDirsEnum.INSTANCE.getAiFilesDirectory();
    }

    public static Path getSslDirectory() {
        return AppDirsEnum.INSTANCE.getSslDirectory();
    }

    private  enum AppDirsEnum {

        INSTANCE(AppDirsFactory.getInstance());

        private final AppDirs appDirs;

        private final Path logDirectoryTemplate;

        private final Path backupDirectory;

        private final Path fulltextIndexBaseDirectory;

        private final Path aiFilesDirectory;

        private final Path sslDirectory;

        AppDirsEnum(AppDirs appDirs) {
            this.appDirs = appDirs;
            this.logDirectoryTemplate = Path.of(appDirs
                    .getUserDataDir(
                            OS.APP_DIR_APP_NAME,
                            "logs",
                            OS.APP_DIR_APP_AUTHOR));
            this.backupDirectory = Path.of(appDirs.getUserDataDir(
                    OS.APP_DIR_APP_NAME,
                    "backups",
                    OS.APP_DIR_APP_AUTHOR));
            this.fulltextIndexBaseDirectory = Path.of(appDirs.getUserDataDir(OS.APP_DIR_APP_NAME,
                    "lucene" + File.separator + LinkedFilesConstants.VERSION,
                    OS.APP_DIR_APP_AUTHOR));
            this.aiFilesDirectory = Path.of(appDirs.getUserDataDir(OS.APP_DIR_APP_NAME,
                    "ai" + File.separator + AiService.VERSION,
                    OS.APP_DIR_APP_AUTHOR));
            this.sslDirectory = Path.of(appDirs.getUserDataDir(OS.APP_DIR_APP_NAME,
                    "ssl",
                    OS.APP_DIR_APP_AUTHOR));
        }

        public AppDirs getAppDirs() {
            return appDirs;
        }

        public Path getLogDirectory(Version version) {
            return this.logDirectoryTemplate.resolve(version.toString());
        }

        public Path getBackupDirectory() {
            return backupDirectory;
        }

        public Path getFulltextIndexBaseDirectory() {
            return fulltextIndexBaseDirectory;
        }

        public Path getAiFilesDirectory() {
            return aiFilesDirectory;
        }

        public Path getSslDirectory() {
            return sslDirectory;
        }

    }

}

@ungerts
Copy link
Contributor

ungerts commented Mar 1, 2025

There may also be a variant that calls AppDirs.getUserDataDir only once.

var appDirs2 = AppDirsFactory.getInstance();
var jabrefUserDataDir = appDirs2.getUserDataDir(OS.APP_DIR_APP_NAME, null, OS.APP_DIR_APP_AUTHOR);
var backupDirectory1 = Path.of(jabrefUserDataDir, "backups");
var aiFilesDirectory1 = Path.of(jabrefUserDataDir, "ai", AiService.VERSION);
var fulltextIndexBaseDirectory1 = Path.of(jabrefUserDataDir, "lucene", LinkedFilesConstants.VERSION.toString());
var sslDirectory1 = Path.of(jabrefUserDataDir, "ssl");

@koppor
Copy link
Member Author

koppor commented Mar 2, 2025

Does an update to the latest release solve the issue, too? 😅 - https://github.com/harawata/appdirs/releases/tag/appdirs-1.4.0

@ungerts
Copy link
Contributor

ungerts commented Mar 2, 2025

I'm not entirely sure, but at least 60 ms are spent when calling mathod com.sun.jna.platform.win32.Guid.GUID#fromString for the first time. This method is part of the JNA (Java Native Access) library.

@koppor
Copy link
Member Author

koppor commented Mar 11, 2025

I'm not entirely sure, but at least 60 ms are spent when calling mathod com.sun.jna.platform.win32.Guid.GUID#fromString for the first time. This method is part of the JNA (Java Native Access) library.

We cannot get rid of this call, can we?

@koppor
Copy link
Member Author

koppor commented Mar 11, 2025

AppDirs was updated at #12671

@koppor koppor added status: waiting-for-feedback The submitter or other users need to provide more information about the issue and removed good first issue An issue intended for project-newcomers. Varies in difficulty. 📍 Assigned Assigned by assign-issue-action (or manually assigned) labels Mar 11, 2025
@koppor
Copy link
Member Author

koppor commented Mar 11, 2025

@lalit2506verma Sorry that this issue took a detour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev: performance status: waiting-for-feedback The submitter or other users need to provide more information about the issue
Projects
None yet
Development

No branches or pull requests

3 participants