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

Feature/query boost #97

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Feature/query boost #97

wants to merge 10 commits into from

Conversation

robertpniewski
Copy link
Owner

@robertpniewski robertpniewski commented Jan 12, 2021

Farewell feature.

Not only raises performance of both simulation preparation phase and the actual simulation, but also has a cool progress status:

image

Tested with all the strategies -- perfect results. Exemplary pictures:

image
image

Pedestrians with lights tested as well, no errors in logs, all perfect:

image

This feature could use another improvement related to real-time in-app no-file caching out-of-zone ways which would improve it even more (!!) but it would require a general revamp of RouteInfo class to hold a WayWithLights set inside and of course a revamp of all routing related classes, so I'm letting it go since the grand release is coming very soon.

Still not satisfied with the refactoring, but it's already decent and it's 4 AM so I'm letting it go as well.

@@ -186,7 +206,24 @@ else if (xmlNode.getNodeName().equals("way")) {
return Optional.of(info);
}

private static RouteInfo parseWayAndNodes(Document nodesViaOverpass, boolean notPedestrian) {
private Optional<RouteInfo> retrieveRouteInfoFromCache(List<Long> osmWayIds, boolean isNotPedestrian) {
RouteInfo info = new RouteInfo();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Code formatting?

OSMElement() {
id = 0;
}
}*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? Did you try to use bus cache after you've done this?
It won't be able to deserialize the entity.

public OSMWay() {
childNodeIds = new ArrayList<>();
isOneWay = false;
}*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? Did you try to use bus cache after you've done this?
It won't be able to deserialize the entity.

logger.info("Operation progress: " + lastBarPosition + "%");
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

File is in not formatted correctly (more than 4 spaces of indentation)

String path = getSimulationDataFileName(zone);
FileWrapper.cacheToFile(data, path);
}

@Override
public ArrayList<RouteNode> getBusRoute(List<OSMWay> route,
Copy link
Collaborator

Choose a reason for hiding this comment

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

File is in not formatted correctly (more than 4 spaces of indentation in some places)

for (String query : waysWithNodesQuerySplit) {
logger.info("Parsing query " + queryNumber++ + "/" + waysWithNodesQuerySplit.size());
ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
double timeLeftFactor = 0.0002;
Copy link
Collaborator

Choose a reason for hiding this comment

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

final?

int queryNumber = 1;
for (String query : waysWithNodesQuerySplit) {
logger.info("Parsing query " + queryNumber++ + "/" + waysWithNodesQuerySplit.size());
ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you spawning a new thread for each query? It adds significant overhead.
Please switch to asynchronous model to send multiple queries and wait for them.

logger.info("Parsing query " + queryNumber++ + "/" + waysWithNodesQuerySplit.size());
ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
double timeLeftFactor = 0.0002;
timeLeft = new AtomicInteger((int) (timeLeftFactor * Math.PI * zone.getRadius() * zone.getRadius()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

You cannot use non-final static variables. That creates huge mess later.
Rearrange the logic, so that you don't have to re-initalize it.
Just init it at the beginning and then set it to specified value each time

waypoints = new ArrayList<>();
for (var p : way.waypoints) {
waypoints.add(p);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not waypoints = new ArrayList<>(way.waypoints) ?

@@ -45,4 +45,6 @@
void parseChildNodesOfWays(Document childNodesOfWays, List<OSMNode> lightsOfTypeA);

Position calculateLatLonBasedOnInternalLights(Node crossroad);

void initializeWayCache(IZone zone, ICacheWrapper wrapper);
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, you do not pass the Singleton dependencies inside the method.
Put the ICacheWrapper into the constructor and the DI will automatically fill it with correct instance.
image

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

Successfully merging this pull request may close these issues.

2 participants