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

Jump to bottom

Replace listeners array with a Map for better performance #4476

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Replace listeners array with a Map for better performance #4476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Replace listeners array with a Map for better performance
  • Loading branch information
@markerikson
markerikson committed Jan 30, 2023
commit 1599aadb204a09e7b43dbb04b66582f056b3c83d
23 changes: 13 additions & 10 deletions src/createStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import {
StoreEnhancer,
Dispatch,
Observer,
ExtendState
ExtendState,
ListenerCallback
} from './types/store'
import { Action } from './types/actions'
import { Reducer } from './types/reducers'
Expand Down Expand Up @@ -119,8 +120,9 @@ export function createStore<S, A extends Action, Ext = {}, StateExt = never>(

let currentReducer = reducer
let currentState = preloadedState as S
let currentListeners: (() => void)[] | null = []
let currentListeners: Map<number, ListenerCallback> | null = new Map()
let nextListeners = currentListeners
let listenerIdCounter = 0
let isDispatching = false

/**
Expand All @@ -132,7 +134,10 @@ export function createStore<S, A extends Action, Ext = {}, StateExt = never>(
*/
function ensureCanMutateNextListeners() {
if (nextListeners === currentListeners) {
nextListeners = currentListeners.slice()
nextListeners = new Map()
currentListeners.forEach((listener, key) => {
nextListeners.set(key, listener)
})
Comment on lines +137 to +140
Copy link

Choose a reason for hiding this comment

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

Why not do this?

Suggested change
nextListeners = new Map()
currentListeners.forEach((listener, key) => {
nextListeners.set(key, listener)
})
nextListeners = new Map(currentListeners)

Copy link
Contributor Author

@markerikson markerikson Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Because in my testing, at least with a Set instead of a Map, .forEach was actually somehow faster 🤷‍♂️ (as mentioned in the original PR description)

Copy link

Choose a reason for hiding this comment

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

Huh, sounds like a bug in the js engine.

}
}

Expand Down Expand Up @@ -197,7 +202,8 @@ export function createStore<S, A extends Action, Ext = {}, StateExt = never>(
let isSubscribed = true

ensureCanMutateNextListeners()
nextListeners.push(listener)
const listenerId = listenerIdCounter++
nextListeners.set(listenerId, listener)

return function unsubscribe() {
if (!isSubscribed) {
Expand All @@ -214,8 +220,7 @@ export function createStore<S, A extends Action, Ext = {}, StateExt = never>(
isSubscribed = false

ensureCanMutateNextListeners()
const index = nextListeners.indexOf(listener)
nextListeners.splice(index, 1)
nextListeners.delete(listenerId)
currentListeners = null
}
}
Expand Down Expand Up @@ -272,11 +277,9 @@ export function createStore<S, A extends Action, Ext = {}, StateExt = never>(
}

const listeners = (currentListeners = nextListeners)
for (let i = 0; i < listeners.length; i++) {
const listener = listeners[i]
listeners.forEach(listener => {
listener()
}

})
return action
}

Expand Down
4 changes: 3 additions & 1 deletion src/types/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ export interface Unsubscribe {
(): void
}

export type ListenerCallback = () => void

declare global {
interface SymbolConstructor {
readonly observable: symbol
Expand Down Expand Up @@ -198,7 +200,7 @@ export interface Store<
* @param listener A callback to be invoked on every dispatch.
* @returns A function to remove this change listener.
*/
subscribe(listener: () => void): Unsubscribe
subscribe(listener: ListenerCallback): Unsubscribe

/**
* Replaces the reducer currently used by the store to calculate the state.
Expand Down