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

Testing performance improvements #504

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

hmottestad
Copy link
Contributor

This is WIP for testing various performance improvements.

First commit contains various small changes to make the code build on my machine and to make it easier for me to test things.

The next commits contain different improvements that make a significant difference to the performance.

@@ -184,7 +184,7 @@ public EndpointStore(EndpointFiles files, HDTOptions spec, boolean inMemDeletes,
throws IOException {
// load HDT file
this.spec = (spec = HDTOptions.ofNullable(spec));
deleteDisabled = spec.getBoolean(OPTION_QENDPOINT_DELETE_DISABLE, false);
deleteDisabled = spec.getBoolean(OPTION_QENDPOINT_DELETE_DISABLE, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must be doing something wrong somewhere. With the HDT and index files I got from Dennis I kept getting an exception related to the part of the code that handles deleted statements. And when I tried to change the config in the application.properties file it didn't seem to make a difference. So I explicitly disabled it in order to be able to test my queries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to set a config, you can add it inside the repo_model.ttl file in the triple

mdlc:main mdlc:hdtSpec "qendpoint.delete.disable=true;" .

By default, we need the deletes, so the default value should be false.

Comment on lines +47 to +90
@Override
public void meet(StatementPattern node) throws RuntimeException {
Var subjectVar = node.getSubjectVar();
if (subjectVar != null && subjectVar.isAnonymous() && subjectVar.hasValue()) {
long id = converter.subjectToID(((Resource) subjectVar.getValue()));
if (id != -1) {
Var var1 = new Var(subjectVar.getName(), converter.idToSubjectHDTResource(id), true,
subjectVar.isConstant());
node.replaceChildNode(subjectVar, var1);
}
}

Var predicateVar = node.getPredicateVar();
if (predicateVar != null && predicateVar.isAnonymous() && predicateVar.hasValue()) {
long id = converter.predicateToID(((IRI) predicateVar.getValue()));
if (id != -1) {
Var var1 = new Var(predicateVar.getName(), converter.idToPredicateHDTResource(id), true,
predicateVar.isConstant());
node.replaceChildNode(predicateVar, var1);
}
}

Var objectVar = node.getObjectVar();
if (objectVar != null && objectVar.isAnonymous() && objectVar.hasValue()) {
long id = converter.objectToID((objectVar.getValue()));
if (id != -1) {
Var var1 = new Var(objectVar.getName(), converter.idToObjectHDTResource(id), true,
objectVar.isConstant());
node.replaceChildNode(objectVar, var1);
}
}

Var contextVar = node.getContextVar();
if (contextVar != null && contextVar.isAnonymous() && contextVar.hasValue()) {
long id = converter.contextToID((((Resource) contextVar.getValue())));
if (id != -1) {
Var var1 = new Var(contextVar.getName(), converter.idToGraphHDTResource(id), true,
contextVar.isConstant());
node.replaceChildNode(contextVar, var1);
}
}

}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the addition where I optimize each var in the StatementPattern. For some reason the code in meet(Var var) didn't manage to make the predicate have the correct position. When working on the StatementPattern we already know which position every var has.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try to use a constant outside a triple to see if it works? For example in a filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tried that. But I assume it would match with the existing meet method for vars. I haven't removed that one, since the meet on the statement pattern doesn't work for things like BIND or FILTER.

Copy link
Collaborator

@ate47 ate47 left a comment

Choose a reason for hiding this comment

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

For me, they are interesting additions. I'm fine with them. Once the points I have mentioned will be fixed, it'll merge it.

const container = document.createElement("pre")

container.innerText = JSON.stringify(plan, null, 2)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you apply the formatter? npm run format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Formatter didn't seem to do anything, but IntelliJ highlighted the issues and I fixed them by hand.

}

}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be reassured with a small test if you can.

Comment on lines +64 to +69
String property = System.getProperty("prototypejoin");
System.err.println("prototypejoin: " + property);
if (property == null || !property.equals("true")) {
System.out.println("prototypejoin is not enabled");
return super.prepare(node, context);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To use the new prototype join:

java -Dprototypejoin=true -jar qendpoint-backend-2.1.2-exec.jar

@ate47
Copy link
Collaborator

ate47 commented Nov 28, 2024

Can I merge it or do you want to add something else?

@hmottestad
Copy link
Contributor Author

Don't merge this one. This is the branch with everything on it. I've created two different branches/PRs with the two sets of changes that we talked about in the meeting.

@hmottestad
Copy link
Contributor Author

You can merge this one here: #527

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants