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

onRelationshipsFlushed, onEntitiesFlushed possible unnecessarily locking? #765

Open
zemberdotnet opened this issue Jul 24, 2022 · 2 comments
Labels
performance question Further information is requested

Comments

@zemberdotnet
Copy link
Member

await this.lockOperation(() =>
pMap(
this.localGraphObjectStore.collectRelationshipsByStep(),
async ([stepId, relationships]) => {
const indexable = relationships.filter((r) => {
const indexMetadata = this.getIndexMetadataForGraphObjectType({
stepId,
_type: r._type,
graphObjectCollectionType: 'relationships',
});
if (typeof indexMetadata === 'undefined') {
return true;
}
return indexMetadata.enabled === true;
});
if (indexable.length) {
await flushDataToDisk({
storageDirectoryPath: stepId,
collectionType: 'relationships',
data: indexable,
pretty: this.prettifyFiles,
});
}
this.localGraphObjectStore.flushRelationships(relationships);
if (onRelationshipsFlushed) {
await onRelationshipsFlushed(relationships);
}
},
),
);

Does the onRelationshipsFlushed hook also need to be locked in the code above? My impression is that after the relationships/entities have been flushed, the hook should be free to run concurrently. If separately a step is running and able to fill the buffer, it must wait until the onRelationshipsFlushed/onEntitiesFlushed hook has finished. If the hook is an upload, that may unnecessarily slow down filling up and flushing the buffer again while uploads are happening. The hook seems like it might be unnecessarily blocking.

@zemberdotnet zemberdotnet added question Further information is requested performance labels Jul 24, 2022
@zemberdotnet
Copy link
Member Author

Thinking about this further, I think that onRelationshipsFlushed/onEntitiesFlushed does not need to be locked. The order of uploads shouldn't matter as long as the uploads are not duplicated. The locking only needs to ensure that the upload isn't duplicated. It can do that by ensuring the buffer is cleared without needing to ensure the upload is finished.

@zemberdotnet
Copy link
Member Author

However, this function would need to be refactored to support that as the reference to the entities or relationships is created during the lock operation itself, but it doesn't escape the block currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance question Further information is requested
Projects
None yet
Development

No branches or pull requests

1 participant