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.

Sign up for GitHub

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

Optimize cache subscriptions #1249

Merged
merged 3 commits into from
Jan 11, 2024
Merged

Optimize cache subscriptions #1249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f50aa7
Optimize cache subscriptions
llJochemll Jan 3, 2024
a87233f
Make prettier happy
llJochemll Jan 3, 2024
4a5fc18
Add changeset
llJochemll Jan 10, 2024
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
5 changes: 5 additions & 0 deletions .changeset/three-comics-greet.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'houdini': patch
---

Optimize cache subscriptions
85 changes: 48 additions & 37 deletions packages/houdini/src/runtime/cache/subscription.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,21 @@ export class InMemorySubscriptions {
this.cache = cache
}

private subscribers: {
[id: string]: { [fieldName: string]: FieldSelection[] }
} = {}
private referenceCounts: {
[id: string]: { [fieldName: string]: Map<SubscriptionSpec['set'], number> }
} = {}
private subscribers = new Map<
string,
Map<
string,
{
selections: FieldSelection[]
referenceCounts: Map<SubscriptionSpec['set'], number>
}
>
>()

private keyVersions: { [key: string]: Set<string> } = {}

activeFields(parent: string): string[] {
return Object.keys(this.subscribers[parent] || {})
return Object.keys(this.subscribers.get(parent) || {})
}

add({
Expand Down Expand Up @@ -137,14 +142,23 @@ export class InMemorySubscriptions {
type: string
}) {
const spec = selection[0]

// if we haven't seen the id or field before, create a list we can add to
if (!this.subscribers[id]) {
this.subscribers[id] = {}
if (!this.subscribers.has(id)) {
this.subscribers.set(id, new Map())
}
if (!this.subscribers[id][key]) {
this.subscribers[id][key] = []

const subscriber = this.subscribers.get(id)!

if (!subscriber.has(key)) {
subscriber.set(key, {
selections: [],
referenceCounts: new Map(),
})
}

const subscriberField = subscriber.get(key)!

// if this is the first time we've seen the raw key
if (!this.keyVersions[key]) {
this.keyVersions[key] = new Set()
Expand All @@ -153,21 +167,15 @@ export class InMemorySubscriptions {
// add this version of the key if we need to
this.keyVersions[key].add(key)

if (!this.subscribers[id][key].map(([{ set }]) => set).includes(spec.set)) {
this.subscribers[id][key].push([spec, selection[1]])
if (!subscriberField.selections.some(([{ set }]) => set === spec.set)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change the core of the improvement? I'm curious if you noticed much of an improvement going from {} to a Map. I'm totally fine to keep it how you have it, just curious

Copy link
Contributor Author

@llJochemll llJochemll Jan 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To answer the Map part first, the performance should be 'better', but on a scale that probably isn't going to matter much. Using Map has advantages, but frankly, my main reason is that the Map api is just nicer to use (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#objects_vs._maps for a comparison)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual improvement comes from combining map() and .includes() into some().

This is because those functions, unlike in let's say C# (IEnumerable) or Java (Stream), operate on arrays, so chaining methods can actually become quite expansive. The reason for that is that map(), filter() and the like return a new array, so you end up allocating n arrays, with n being the amount of chained functions.

This doesn't matter that much for small arrays, but for larger arrays it starts to add up.

subscriberField.selections.push([spec, selection[1]])
}

// if this is the first time we've seen this key
if (!this.referenceCounts[id]) {
this.referenceCounts[id] = {}
}
if (!this.referenceCounts[id][key]) {
this.referenceCounts[id][key] = new Map()
}
const counts = this.referenceCounts[id][key]

// we're going to increment the current value by one
counts.set(spec.set, (counts.get(spec.set) || 0) + 1)
subscriberField.referenceCounts.set(
spec.set,
(subscriberField.referenceCounts.get(spec.set) || 0) + 1
)

// reset the lifetime for the key
this.cache._internal_unstable.lifetimes.resetLifetime(id, key)
Expand Down Expand Up @@ -293,7 +301,7 @@ export class InMemorySubscriptions {
}

get(id: string, field: string): FieldSelection[] {
return this.subscribers[id]?.[field] || []
return this.subscribers.get(id)?.get(field)?.selections || []
}

remove(
Expand Down Expand Up @@ -346,18 +354,16 @@ export class InMemorySubscriptions {

reset() {
// Get all subscriptions that do not start with the rootID
const subscribers = Object.entries(this.subscribers).filter(
([id]) => !id.startsWith(rootID)
)
const subscribers = [...this.subscribers.entries()].filter(([id]) => !id.startsWith(rootID))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, doesn't .entries() return a new list? It's probably neglible but it'd be great to avoid the shallow copy if possible.

same with the .values below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So .entries() returns a new iterator, not an array. The spread operator than converts the iterator to an array to be able to use filter on it.

So while Object.entries() returns an array, Map.entries() returns a reference that contains the information on how to build the array (grossly simplified), and the spread operator uses it to then actually do it. In both cases only one array is actually constructed.


// Remove those subcribers from this.subscribers
for (const [id, _fields] of subscribers) {
delete this.subscribers[id]
this.subscribers.delete(id)
}

// Get list of all SubscriptionSpecs of subscribers
const subscriptionSpecs = subscribers.flatMap(([_id, fields]) =>
Object.values(fields).flatMap((field) => field.map(([spec]) => spec))
[...fields.values()].flatMap((field) => field.selections.map(([spec]) => spec))
)

return subscriptionSpecs
Expand All @@ -368,12 +374,16 @@ export class InMemorySubscriptions {
// checking reference counts
let targets: SubscriptionSpec['set'][] = []

const subscriber = this.subscribers.get(id)
const subscriberField = subscriber?.get(fieldName)

for (const spec of specs) {
const counts = subscriber?.get(fieldName)?.referenceCounts

// if we dont know this field/set combo, there's nothing to do (probably a bug somewhere)
if (!this.referenceCounts[id]?.[fieldName]?.has(spec.set)) {
if (!counts?.has(spec.set)) {
continue
}
const counts = this.referenceCounts[id][fieldName]
const newVal = (counts.get(spec.set) || 0) - 1

// decrement the reference of every field
Expand All @@ -387,8 +397,8 @@ export class InMemorySubscriptions {
}

// we do need to remove the set from the list
if (this.subscribers[id]) {
this.subscribers[id][fieldName] = this.get(id, fieldName).filter(
if (subscriberField) {
subscriberField.selections = this.get(id, fieldName).filter(
([{ set }]) => !targets.includes(set)
)
}
Expand All @@ -397,17 +407,18 @@ export class InMemorySubscriptions {
removeAllSubscribers(id: string, targets?: SubscriptionSpec[], visited: string[] = []) {
visited.push(id)

const subscriber = this.subscribers.get(id)
// every field that currently being subscribed to needs to be cleaned up
for (const field of Object.keys(this.subscribers[id] || [])) {
for (const [key, val] of subscriber?.entries() ?? []) {
// grab the current set of subscribers
const subscribers = targets || this.subscribers[id][field].map(([spec]) => spec)
const subscribers = targets || val.selections.map(([spec]) => spec)

// delete the subscriber for the field
this.removeSubscribers(id, field, subscribers)
this.removeSubscribers(id, key, subscribers)

// look up the value for the field so we can remove any subscribers that existed because of a
// subscriber to this record
const { value, kind } = this.cache._internal_unstable.storage.get(id, field)
const { value, kind } = this.cache._internal_unstable.storage.get(id, key)

// if the field is a scalar, there's nothing more to do
if (kind === 'scalar') {
Expand Down
Loading