[FIXED] Kann der synchronisierte innere Block die Leistung einer bereits synchronisierten Methode verbessern?

Ausgabe

Gegeben sei ein theoretisches System, in dem Dateien aus dem Web heruntergeladen werden, wenn sie nicht im lokalen System gefunden werden, und unter der Annahme:

1. Der Download-Mechanismus und das Abrufen/Platzieren aus dem/im Cache (lokales Dateisystem) sind bereits erledigt.
2. Single-Threaded und einzelne Anfrage pro URL.

Ich habe eine Methode geschrieben, die getFileFromLocalFS() und getFileFromWeb() verwendet, um eine vereinfachte Caching-Logik zu implementieren:

public InputStream getFile(String url) { // #1
    InputStream retStream = getFileFromLocalFS(url);
    if (retStream != null) {
        return retStream;           
    }
    else {
        retStream = getFileFromLocalFS(url);
        if (retStream == null) {
            return getFileFromWeb(url);
        }
    }
    return retStream;
}

Diese schematische Lösung musste dann verbessert werden, um gleichzeitige Anforderungen zum Herunterladen von derselben URL zu berücksichtigen … und das tatsächliche “aus dem Web” auf einen einzigen Download zu beschränken (dh alle anderen Anforderungen erhalten ihn vom lokalen Dateisystem). Also habe ich die gesamte Methode synchronisiert:

public synchronized InputStream getFile(String url) { // #2
    InputStream retStream = getFileFromLocalFS(url);
    if (retStream != null) {
        return retStream;           
    }
    else {
        retStream = getFileFromLocalFS(url);
        if (retStream == null) {
            return getFileFromWeb(url);
        }
    }
    return retStream;
}

Dies erfüllt im Wesentlichen die Anforderung, hat jedoch ein Leistungsproblem, da verhindert wird, dass die gesamte Methode von einem anderen Thread ausgeführt wird, bis sie abgeschlossen ist. Das heißt, selbst wenn die Datei vom lokalen FS abgerufen werden kann, getFileFromLocalFS(url)kann nicht darauf zugegriffen werden, während die Methode von einem anderen Thread ausgeführt wird.

Eine von meinem Interviewer vorgeschlagene Leistungsverbesserung bestand darin, den getFileFromLocalFS(url)Block zu synchronisieren:

public synchronized InputStream getFile(String url) { // #3
    InputStream retStream = getFileFromLocalFS(url);
    if (retStream != null) {
        return retStream;           
    }
    else {
        synchronized (this) { 
            retStream = getFileFromLocalFS(url);
            if (retStream == null) {
                return getFileFromWeb(url);
            }
        }
    }
    return retStream;
}

Ich sagte “gut, aber damit diese Optimierung funktioniert, muss die Methodensynchronisierung entfernt werden”, dh:

public InputStream getFile(String url) { // #4
    InputStream retStream = getFileFromLocalFS(url);
    if (retStream != null) {
        return retStream;           
    }
    else {
        synchronized (this) { 
            retStream = getFileFromLocalFS(url);
            if (retStream == null) {
                return getFileFromWeb(url);
            }
        }
    }
    return retStream;
}

Der Interviewer war anderer Meinung und bestand darauf, beides synchronized an Ort und Stelle zu lassen.

Welche würde in einer Parallelitätsumgebung besser abschneiden? #3 oder #4? Wieso den?

Lösung

Es scheint Cargo Cult Programming zu geben. Die dritte Variante ähnelt Double-Checked Locking , kommt aber nicht auf den Punkt. Aber noch auffälliger ist, dass die erste Variante auch Double-Checked Locking ähnelt, indem sie getFileFromLocalFS(url)ohne Grund zweimal anruft. Und das ist der Ausgangspunkt …

Ihr Verdacht gegen die „Verbesserung“ des Interviewers ist richtig. Verschachtelte Synchronisierungen haben keinen Zweck und im besten Fall kann der Optimierer der JVM sie beseitigen.

Beide Lösungen werden jedoch nicht empfohlen. Hier gibt es ein viel tieferes Problem. Zuerst sollten wir uns auf die Korrektheit konzentrieren , bevor wir die Leistung diskutieren.

Apparently, getFileFromLocalFS(url) is supposed to find a local copy if it exists, but not to create it. This would be pointless if no-one ever creates such copies and the whole question about overlapping caching attempts would be moot, if this method doesn’t perform the caching attempt. So the only other method involved, getFileFromWeb(url) must be the one which creates the local copy.

This implies that there is a hidden protocol between these two methods. getFileFromLocalFS(url) somehow knows, how to get the cached file created by getFileFromWeb(url).

This raises questions, like, what happens if one thread is in the middle of getFileFromWeb(url) creating a cached version and another thread calls getFileFromLocalFS(url) for the same url? Does it return an input stream to the still incomplete local file? There are two possibilities:

1. This issue has been solved somehow. This would imply that there is already some thread safe construct behind the scenes to prevent such a race condition, hence, it should be used by getFileFromWeb(url) to also solve the issue of multiple caching attempts in the first place.

2. Or this issue wasn’t solved and the starting point was a broken method, so the first task should be to fix this, instead of trying to improve the performance. If we really try to fix this outside the two methods, i.e. in getFile, only the second variant would be correct, calling both methods inside synchronized.

In either case, the getFile method is the wrong place to address the issue. The concurrency issues should be addressed by the hidden protocol that already exists between the two methods.

Zum Beispiel:

• Wenn diese beiden Methoden eine Zuordnung von URL zu lokaler Datei verwenden, sollten sie eine gleichzeitige Zuordnung und ihre atomaren Aktualisierungsvorgänge verwenden, um die Parallelitätsprobleme zu lösen.

• Wenn diese beiden Methoden ein Zuordnungsschema verwenden, um die URL in einen lokalen Pfad zu konvertieren, in dem sich die Kopie befinden soll, und die getFileFromLocalFS(url)Methode lediglich das Vorhandensein der Datei überprüft, getFileFromWeb(url)sollte die Methode die Datei mit der CREATE_NEWOption erstellen, das Vorhandensein atomar zu überprüfen und erstellen, falls nicht vorhanden, zusammen mit Dateisperren, um zu verhindern, dass andere Threads lesen, während das Caching noch im Gange ist.

Beantwortet von – Holger

Antwort geprüft von – Dawn Plyler (FixError Volunteer)