Perf optimisations#98

DZakh · 2024-07-31T08:04:02Z

No description provided.

DZakh · 2024-07-31T08:06:08Z

 let mapArrayOfResults = (results: array<result<'a, 'b>>): result<array<'a>, 'b> => {
-  results->Belt.Array.reduce(Ok([]), (accum, nextItem) => {
-    accum->Belt.Result.flatMap(currentOkItems => {
-      nextItem->Belt.Result.map(item => Belt.Array.concat(currentOkItems, [item]))
-    })
-  })
+  let rec loop = (index: int, output: array<'a>): result<array<'a>, 'b> => {
+    if index >= Array.length(results) {
+      Ok(output)
+    } else {
+      switch results->Js.Array2.unsafe_get(index) {
+      | Ok(value) => {
+          output[index] = value
+          loop(index + 1, output)
+        }
+      | Error(_) as err => err->(magic: result<'a, 'b> => result<array<'a>, 'b>)
+      }
+    }
+  }
+
+  loop(0, Belt.Array.makeUninitializedUnsafe(results->Js.Array2.length))
 }


@JonoPrest Noticed the code, and it literally looks the same as a redux selector from the messaging app I was talking you about 😁

Here's the benchmark results for running the function with 10000 Ok items:

Super nice!

DZakh · 2024-07-31T08:06:43Z

-      if entityIdsMap->EntityIdsMap.hasId(~entityName=EntityMod.name, ~entityId) {
-        inMemTable->InMemoryTable.Entity.get(entityId)
-      } else {
-        // NOTE: this will still return the value if it exists in the in-memory store (despite the loader not being run).
-        switch inMemTable->InMemoryTable.Entity.get(entityId) {
-        | Some(entity) => Some(entity)
-        | None =>
-          let entities = await asyncGetter(entityId)
-
-          let optEntity = entities->Belt.Array.get(0)
-          inMemTable->InMemoryTable.Entity.initValue(~key=entityId, ~entity=optEntity)
-
-          optEntity
-        }
+      switch inMemTable->InMemoryTable.Entity.get(entityId) {
+      | Some(entity) => Some(entity)
+      | None =>
+        let entities = await asyncGetter(entityId)
+
+        let optEntity = entities->Belt.Array.get(0)
+        inMemTable->InMemoryTable.Entity.initValue(~key=entityId, ~entity=optEntity)
+
+        optEntity


This is the only place where it's used, which doesn't look relevant after we migrated to async handlers.

JonoPrest · 2024-07-31T08:17:19Z

+          output[index] = value
+          loop(index + 1, output)
+        }
+      | Error(_) as err => err->(magic: result<'a, 'b> => result<array<'a>, 'b>)


Why not simply do this? Are you trying to reduce overhead with the magic?

Suggested change

| Error(_) as err => err->(magic: result<'a, 'b> => result<array<'a>, 'b>)

| Error(e) => Error(e)

It's a tiny optimisation to avoid recreating the {TAG: "Error", _0: 'a} object

JonoPrest · 2024-07-31T08:18:39Z

  logger: Pino.t,
  chain: ChainMap.Chain.t,
  addedDynamicContractRegistrations: array<TablesStatic.DynamicContractRegistry.t>,
-  entityIdsMap: EntityIdsMap.t,


@DZakh this is still used/useful so I would say don't remove it but there may be a nicer way to achieve this that doesn't involve another map?

JonoPrest · 2024-07-31T08:21:18Z

-      if entityIdsMap->EntityIdsMap.hasId(~entityName=EntityMod.name, ~entityId) {
-        inMemTable->InMemoryTable.Entity.get(entityId)
-      } else {
-        // NOTE: this will still return the value if it exists in the in-memory store (despite the loader not being run).
-        switch inMemTable->InMemoryTable.Entity.get(entityId) {
-        | Some(entity) => Some(entity)
-        | None =>
-          let entities = await asyncGetter(entityId)


We can't remove this because it protects against round trips to the db when a value is confirmed doesn't exist.

I think we can probably optimize by first looking in the in mem table and only checking if it was previously looked up before the async call. Not sure how much that would help though.

JonoPrest

Love the util optimization! But we shouldn't remove the other code without another solution 👍🏼

DZakh · 2024-07-31T08:35:55Z

+  // It returns option<option<'entity>> where the first option means
+  // that the entity is not set to the in memory store,
+  // and the second option means that the entity doesn't esist/deleted.
+  // It's needed to prevent an additional round trips to the database for deleted entities.
  let get = (inMemTable: t<'entity>, key: Types.id) =>
    inMemTable.table
    ->get(key)
-    ->Option.flatMap(rowToEntity)
+    ->Option.map(rowToEntity)


This should also improve these lines in LoaderLayer:

//filter out ids that don't already exist in the in memory store let idsNotInMemory = entityLoadIds->Array.keep(id => inMemTable->InMemoryTable.Entity.get(id)->Option.isNone) //load in values that don't exist in the inMemoryStore let res = await idsNotInMemory->batchLoadIds

JonoPrest · 2024-07-31T08:54:49Z

-        | Some(entity) => resolve(Some(entity))
-        | None => resolve(None)
+        | Some(maybeEntity) => resolve(maybeEntity)
+        | None => resolve(None) // Shouldn't happen


Why should this not happen? (Could also use your flatten Util here)

Since we should load all entities a few lines above

Yes but lets say, "idsNotInMemory" is [1, 2, 3] and we batch load those ids. "res" could be [{id: 1, ..}, {id: 3, ..}] for example skipping values that didn't exist in the db.

Then when you iterate over that they don't get initialized in the in memory store.

So I guess for this assumption to be correct you need to initialize all values that weren't returned from the DB as NotSet or something

Hm, sounds like something to improve in the future.

Co-authored-by: Jono Prest <65739024+JonoPrest@users.noreply.github.com>

JonoPrest · 2024-07-31T12:42:54Z

Hey @DZakh, I like the improvements and moving to static but looks like we still need to solve for the regression of adding round trips to the DB when they have already been requested in loaders.

Maybe as a suggestion instead of "idsNotInMemory" being an array. Make it a set, and on the response remove ids from the set as you initialize values. Then with the remaining set items initialize them as NotSet?

JonoPrest

Awesome stuff @DZakh looks great 👍🏼

DZakh added 2 commits July 31, 2024 11:56

Refactor mapArrayOfResults for ~4000x speed improvement

37e90d5

Remove entityIdsMap

1fcae72

DZakh commented Jul 31, 2024

View reviewed changes

JonoPrest reviewed Jul 31, 2024

View reviewed changes

JonoPrest requested changes Jul 31, 2024

View reviewed changes

Prevent db roundtrip on not-existing/deleted entity in memory store

a90c0db

DZakh commented Jul 31, 2024

View reviewed changes

DZakh requested a review from JonoPrest July 31, 2024 08:37

JonoPrest reviewed Jul 31, 2024

View reviewed changes

Comment thread codegenerator/cli/templates/static/codegen/src/InMemoryTable.res Outdated

JonoPrest reviewed Jul 31, 2024

View reviewed changes

DZakh and others added 2 commits July 31, 2024 13:00

Update codegenerator/cli/templates/static/codegen/src/InMemoryTable.res

a6356e0

Co-authored-by: Jono Prest <65739024+JonoPrest@users.noreply.github.com>

Fix after review

13f5f02

DZakh requested a review from JonoPrest July 31, 2024 09:27

Move LoadLayer to static codegen

cfa9f64

Fix round trip on loaded missing entities

7d8a98e

JonoPrest approved these changes Aug 1, 2024

View reviewed changes

Merge branch 'main' into dz/perf-optimisations

3eb531d

DZakh merged commit 27566cc into main Aug 1, 2024

DZakh deleted the dz/perf-optimisations branch August 1, 2024 09:27

	\| Error(_) as err => err->(magic: result<'a, 'b> => result<array<'a>, 'b>)
	\| Error(e) => Error(e)

Uh oh!

Conversation

DZakh commented Jul 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest commented Jul 31, 2024

Uh oh!

JonoPrest left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JonoPrest Jul 31, 2024 •

edited

Loading