mihon icon indicating copy to clipboard operation
mihon copied to clipboard
verified publisher icon mihonapp

Run `findChapterDir` in parallel

Open FooIbar opened this issue 1 year ago • 10 comments

Follow-up of #728

FooIbar avatar May 01 '24 05:05 FooIbar

Any reason to not use async?

AntsyLich avatar May 01 '24 07:05 AntsyLich

Any reason to not use async?

It does use async under the hood. https://github.com/arrow-kt/arrow/blob/2860c124f9628af4cdf1cb2bddae2f7484697cf4/arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/ParMap.kt#L33

FooIbar avatar May 01 '24 07:05 FooIbar

Yeah why not just use async directly?

AntsyLich avatar May 01 '24 07:05 AntsyLich

Yeah why not just use async directly?

I don't get what you mean, can you elaborate on that?

FooIbar avatar May 01 '24 07:05 FooIbar

Yeah why not just use async directly?

I don't get what you mean, can you elaborate on that?

Why pull in arrow for a very simple function that is feasible to write/copy in Mihon with normal Kotlin coroutines?

Edit: it looks like it's literally just a map of asyncs that are awaited, and then notNull filtered

MajorTanya avatar May 01 '24 07:05 MajorTanya

Yeah why not just use async directly?

I don't get what you mean, can you elaborate on that?

Why pull in arrow for a very simple function that is feasible to write/copy in Mihon with normal Kotlin coroutines?

Edit: it looks like it's literally just a map of asyncs that are awaited, and then notNull filtered

Why reinvent the wheel? And R8 will take care of the unused code.

FooIbar avatar May 01 '24 08:05 FooIbar

Why reinvent the wheel?

Just because you have to write a little more to replicate the same thing as a helper method in a third-party library doesn't mean you have reinvented the wheel.

ghostbear avatar May 01 '24 10:05 ghostbear

Why reinvent the wheel?

Just because you have to write a little more to replicate the same thing as a helper method in a third-party library doesn't mean you have reinvented the wheel.

w/e, this PR is just a POC and anyone can take the idea and reimplement it in their desired way if they want.

FooIbar avatar May 01 '24 10:05 FooIbar

As requested in https://github.com/mihonapp/mihon/issues/705#issuecomment-2088015058, I have run a benchmark similar to the one in https://github.com/mihonapp/mihon/issues/705#issuecomment-2087787294 on the current main branch (including now https://github.com/mihonapp/mihon/pull/728) with and without this PR on top.

For downloading 50 new chapters with 1,000 chapters already downloaded, and measuring the time from clicking the download button (with 50 chapters selected) to the time that the first HTTP request is started:

  • Without this PR: 5.29s
  • With this PR: 2.18s

So it does seem to be a substantial performance improvement.

There may be a bug though, I opened the download queue while some downloads were executing and received the following crash which seems potentially related to the changes in this PR:

java.lang.Throwable: java.util.ConcurrentModificationException
	at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:797)
	at java.util.LinkedHashMap$LinkedKeyIterator.next(LinkedHashMap.java:819)
	at kotlinx.serialization.internal.CollectionLikeSerializer.serialize(CollectionSerializers.kt:69)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.encodeSerializableValue(ProtobufEncoding.kt:138)
	at kotlinx.serialization.protobuf.internal.ProtobufTaggedEncoder.encodeSerializableElement(ProtobufTaggedEncoder.kt:135)
	at eu.kanade.tachiyomi.data.download.MangaDirectory.write$Self$app_devDebug(DownloadCache.kt:438)
	at eu.kanade.tachiyomi.data.download.MangaDirectory$$serializer.serialize(DownloadCache.kt:438)
	at eu.kanade.tachiyomi.data.download.MangaDirectory$$serializer.serialize(DownloadCache.kt:438)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.encodeSerializableValue(ProtobufEncoding.kt:138)
	at kotlinx.serialization.protobuf.internal.ProtobufTaggedEncoder.encodeSerializableElement(ProtobufTaggedEncoder.kt:135)
	at kotlinx.serialization.internal.KeyValueSerializer.serialize(Tuples.kt:31)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.encodeSerializableValue(ProtobufEncoding.kt:138)
	at kotlinx.serialization.protobuf.internal.ProtobufTaggedEncoder.encodeSerializableElement(ProtobufTaggedEncoder.kt:135)
	at kotlinx.serialization.internal.CollectionLikeSerializer.serialize(CollectionSerializers.kt:69)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.serializeMap(ProtobufEncoding.kt:155)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.encodeSerializableValue(ProtobufEncoding.kt:135)
	at kotlinx.serialization.protobuf.internal.ProtobufTaggedEncoder.encodeSerializableElement(ProtobufTaggedEncoder.kt:135)
	at eu.kanade.tachiyomi.data.download.SourceDirectory.write$Self$app_devDebug(DownloadCache.kt:428)
	at eu.kanade.tachiyomi.data.download.SourceDirectory$$serializer.serialize(DownloadCache.kt:428)
	at eu.kanade.tachiyomi.data.download.SourceDirectory$$serializer.serialize(DownloadCache.kt:428)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.encodeSerializableValue(ProtobufEncoding.kt:138)
	at kotlinx.serialization.protobuf.internal.ProtobufTaggedEncoder.encodeSerializableElement(ProtobufTaggedEncoder.kt:135)
	at kotlinx.serialization.internal.KeyValueSerializer.serialize(Tuples.kt:31)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.encodeSerializableValue(ProtobufEncoding.kt:138)
	at kotlinx.serialization.protobuf.internal.ProtobufTaggedEncoder.encodeSerializableElement(ProtobufTaggedEncoder.kt:135)
	at kotlinx.serialization.internal.CollectionLikeSerializer.serialize(CollectionSerializers.kt:69)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.serializeMap(ProtobufEncoding.kt:155)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.encodeSerializableValue(ProtobufEncoding.kt:135)
	at kotlinx.serialization.protobuf.internal.ProtobufTaggedEncoder.encodeSerializableElement(ProtobufTaggedEncoder.kt:135)
	at eu.kanade.tachiyomi.data.download.RootDirectory.write$Self$app_devDebug(DownloadCache.kt:418)
	at eu.kanade.tachiyomi.data.download.RootDirectory$$serializer.serialize(DownloadCache.kt:418)
	at eu.kanade.tachiyomi.data.download.RootDirectory$$serializer.serialize(DownloadCache.kt:418)
	at kotlinx.serialization.protobuf.internal.ProtobufEncoder.encodeSerializableValue(ProtobufEncoding.kt:138)
	at kotlinx.serialization.protobuf.ProtoBuf.encodeToByteArray(ProtoBuf.kt:137)
	at eu.kanade.tachiyomi.data.download.DownloadCache$updateDiskCache$1.invokeSuspend(DownloadCache.kt:465)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:104)
	at kotlinx.coroutines.internal.LimitedDispatcher$Worker.run(LimitedDispatcher.kt:111)
	at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:99)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:585)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:802)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:706)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:693)
	Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@1a39b91, Dispatchers.IO]

raxod502 avatar May 04 '24 21:05 raxod502

There may be a bug though, I opened the download queue while some downloads were executing and received the following crash which seems potentially related to the changes in this PR:

Interesting, but that does not seem to be caused by this PR, we don't update download cache here. rootDownloadsDirLock should be acquired here https://github.com/mihonapp/mihon/blob/dbcc4a7d714cf36b48a41d6074ef671f3c039823/app/src/main/java/eu/kanade/tachiyomi/data/download/DownloadCache.kt#L400

FooIbar avatar May 05 '24 01:05 FooIbar