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

perf: Execute patches in parallel #321

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/main/kotlin/app/revanced/patcher/Fingerprint.kt
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,11 @@ class Fingerprint internal constructor(
}
if (match != null) return match

classes.forEach { classDef ->
match = matchOrNull(classDef)
if (match != null) return match
synchronized(classes) {
classes.forEach { classDef ->
match = matchOrNull(classDef)
if (match != null) return match
}
}

return null
Expand Down
162 changes: 98 additions & 64 deletions src/main/kotlin/app/revanced/patcher/Patcher.kt
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package app.revanced.patcher

import app.revanced.patcher.patch.*
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.*
import kotlinx.coroutines.flow.channelFlow
import java.io.Closeable
import java.util.concurrent.ConcurrentHashMap
import java.util.logging.Logger

/**
Expand Down Expand Up @@ -56,41 +58,7 @@ class Patcher(private val config: PatcherConfig) : Closeable {
*
* @return A flow of [PatchResult]s.
*/
operator fun invoke() = flow {
fun Patch<*>.execute(
executedPatches: LinkedHashMap<Patch<*>, PatchResult>,
): PatchResult {
// If the patch was executed before or failed, return it's the result.
executedPatches[this]?.let { patchResult ->
patchResult.exception ?: return patchResult

return PatchResult(this, PatchException("The patch '$this' failed previously"))
}

// Recursively execute all dependency patches.
dependencies.forEach { dependency ->
dependency.execute(executedPatches).exception?.let {
return PatchResult(
this,
PatchException(
"The patch \"$this\" depends on \"$dependency\", which raised an exception:\n${it.stackTraceToString()}",
),
)
}
}

// Execute the patch.
return try {
execute(context)

PatchResult(this)
} catch (exception: PatchException) {
PatchResult(this, exception)
} catch (exception: Exception) {
PatchResult(this, PatchException(exception))
}.also { executedPatches[this] = it }
}

operator fun invoke() = channelFlow {
// Prevent decoding the app manifest twice if it is not needed.
if (config.resourceMode != ResourcePatchContext.ResourceMode.NONE) {
context.resourceContext.decodeResources(config.resourceMode)
Expand All @@ -103,48 +71,114 @@ class Patcher(private val config: PatcherConfig) : Closeable {

logger.info("Executing patches")

val executedPatches = LinkedHashMap<Patch<*>, PatchResult>()
val executedPatches = ConcurrentHashMap<Patch<*>, Deferred<PatchResult>>()

context.executablePatches.sortedBy { it.name }.forEach { patch ->
val patchResult = patch.execute(executedPatches)
suspend operator fun Patch<*>.invoke(): Deferred<PatchResult> {
val patch = this

// If an exception occurred or the patch has no finalize block, emit the result.
if (patchResult.exception != null || patch.finalizeBlock == null) {
emit(patchResult)
// If the patch was executed before or failed, return it's the result.
executedPatches[patch]?.let { deferredPatchResult ->
val patchResult = deferredPatchResult.await()

patchResult.exception ?: return deferredPatchResult

return CompletableDeferred(PatchResult(patch, PatchException("The patch '$patch' failed previously")))
}
}

val succeededPatchesWithFinalizeBlock = executedPatches.values.filter {
it.exception == null && it.patch.finalizeBlock != null
}
return async(Dispatchers.IO) {
// Recursively execute all dependency patches.
val dependenciesResult = coroutineScope {
val dependenciesJobs = dependencies.map { dependency ->
async(Dispatchers.IO) {
dependency().await().exception?.let { exception ->
PatchResult(
patch,
PatchException(
"""
The patch "$patch" depends on "$dependency", which raised an exception:
${exception.stackTraceToString()}
""".trimIndent(),
),
)
}
}
}

dependenciesJobs.awaitAll().firstOrNull { result -> result != null }?.let {
dependenciesJobs.forEach(Deferred<*>::cancel)

return@coroutineScope it
}
}

succeededPatchesWithFinalizeBlock.asReversed().forEach { executionResult ->
val patch = executionResult.patch
if (dependenciesResult != null) {
return@async dependenciesResult
}

val result =
// Execute the patch.
try {
patch.finalize(context)
execute(context)

executionResult
PatchResult(patch)
} catch (exception: PatchException) {
PatchResult(patch, exception)
} catch (exception: Exception) {
PatchResult(patch, PatchException(exception))
}
}.also { executedPatches[patch] = it }
}

if (result.exception != null) {
emit(
PatchResult(
patch,
PatchException(
"The patch \"$patch\" raised an exception: ${result.exception.stackTraceToString()}",
result.exception,
),
),
)
} else if (patch in context.executablePatches) {
emit(result)
}
coroutineScope {
context.executablePatches.sortedBy { it.name }.map { patch ->
launch(Dispatchers.IO) {
val patchResult = patch().await()

// If an exception occurred or the patch has no finalize block, emit the result.
if (patchResult.exception != null || patch.finalizeBlock == null) {
send(patchResult)
}
}
}.joinAll()
}

val succeededPatchesWithFinalizeBlock = executedPatches.values.map { it.await() }.filter {
it.exception == null && it.patch.finalizeBlock != null
}

coroutineScope {
succeededPatchesWithFinalizeBlock.asReversed().map { executionResult ->
launch(Dispatchers.IO) {
val patch = executionResult.patch

val result =
try {
patch.finalize(context)

executionResult
} catch (exception: PatchException) {
PatchResult(patch, exception)
} catch (exception: Exception) {
PatchResult(patch, PatchException(exception))
}

if (result.exception != null) {
send(
PatchResult(
patch,
PatchException(
"""
The patch "$patch" raised an exception during finalization:
${result.exception.stackTraceToString()}
""".trimIndent(),
result.exception,
),
),
)
} else if (patch in context.executablePatches) {
send(result)
}
}
}.joinAll()
}
}

Expand Down
30 changes: 25 additions & 5 deletions src/main/kotlin/app/revanced/patcher/patch/BytecodePatchContext.kt
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,23 @@ class BytecodePatchContext internal constructor(private val config: PatcherConfi
* @param predicate A predicate to match the class.
* @return A proxy for the first class that matches the predicate.
*/
fun classBy(predicate: (ClassDef) -> Boolean) =
classes.proxyPool.find { predicate(it.immutableClass) } ?: classes.find(predicate)?.let { proxy(it) }
fun classBy(predicate: (ClassDef) -> Boolean): ClassProxy? {
synchronized(classes.proxyPool) {
val proxy = classes.proxyPool.find { predicate(it.immutableClass) }
if (proxy != null) return proxy;
}

val classDef: ClassDef?
synchronized(classes) {
Copy link
Member Author

@oSumAtrIX oSumAtrIX Nov 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is synchronization needed here? Patches can also access the classes list, should they also synchronize? Would it not be possible to use a ConcurrentList so that we don't have to synchronize everytime the classes list is accessed?

Copy link
Contributor

@LisoUseInAIKyrios LisoUseInAIKyrios Nov 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iteration is not thread safe if another piece of code can change the list.

If there is no way the list can ever be changed after it is created then synchronization is not needed here.

classDef = classes.find(predicate)
}

if (classDef != null) {
return proxy(classDef)
}

return null
}

/**
* Proxy the class to allow mutation.
Expand All @@ -108,9 +123,14 @@ class BytecodePatchContext internal constructor(private val config: PatcherConfi
*
* @return A proxy for the class.
*/
fun proxy(classDef: ClassDef) = classes.proxyPool.find {
it.immutableClass.type == classDef.type
} ?: ClassProxy(classDef).also { classes.proxyPool.add(it) }
fun proxy(classDef: ClassDef) : ClassProxy {
val proxyPool = classes.proxyPool
synchronized(proxyPool) {
return proxyPool.find {
it.immutableClass.type == classDef.type
} ?: ClassProxy(classDef).also { proxyPool.add(it) }
}
}

/**
* Navigate a method.
Expand Down
26 changes: 15 additions & 11 deletions src/main/kotlin/app/revanced/patcher/util/ProxyClassList.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,32 @@ package app.revanced.patcher.util

import app.revanced.patcher.util.proxy.ClassProxy
import com.android.tools.smali.dexlib2.iface.ClassDef
import java.util.*

/**
* A list of classes and proxies.
*
* @param classes The classes to be backed by proxies.
*/
class ProxyClassList internal constructor(classes: MutableList<ClassDef>) : MutableList<ClassDef> by classes {
internal val proxyPool = mutableListOf<ClassProxy>()
class ProxyClassList internal constructor(classes: MutableList<ClassDef>) : MutableList<ClassDef> by Collections.synchronizedList(classes) {
internal val proxyPool = Collections.synchronizedList(mutableListOf<ClassProxy>())

/**
* Replace all classes with their mutated versions.
*/
internal fun replaceClasses() =
proxyPool.removeIf { proxy ->
// If the proxy is unused, return false to keep it in the proxies list.
if (!proxy.resolved) return@removeIf false
internal fun replaceClasses() {
synchronized(proxyPool) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem necessary. The function is called after all patches have executed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency it can be synchronized on iteration, just to have the same behavior everywhere. There is no performance overhead of doing this.

proxyPool.removeIf { proxy ->
// If the proxy is unused, return false to keep it in the proxies list.
if (!proxy.resolved) return@removeIf false

// If it has been used, replace the original class with the mutable class.
remove(proxy.immutableClass)
add(proxy.mutableClass)
// If it has been used, replace the original class with the mutable class.
remove(proxy.immutableClass)
add(proxy.mutableClass)

// Return true to remove the proxy from the proxies list.
return@removeIf true
// Return true to remove the proxy from the proxies list.
return@removeIf true
}
}
}
}
6 changes: 3 additions & 3 deletions src/test/kotlin/app/revanced/patcher/PatcherTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ internal object PatcherTest {
patches()

assertEquals(
listOf("1", "2", "3", "4", "-1", "-2"),
executed,
"Expected patches to be executed in correct order.",
setOf("1", "2", "3", "4", "-1", "-2"),
executed.toSet(),
"Unexpected patch results",
)
}

Expand Down
Loading