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

Optimize performance for the list request /api/datasets #3441

Merged
merged 9 commits into from
Nov 22, 2018

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Nov 9, 2018

Steps to test:

  • get yourself a setup with many datasets (find a postgres dump in my home dir on botanix, importable with psql postgresql://postgres:postgres@localhost:5432/webknossos < dump-2018-11-09.sql)
  • request http://localhost:9000/api/datasets
  • For me this takes about 8–10 seconds with the master code, and about 4–6 seconds with this change

Issues:


@fm3 fm3 self-assigned this Nov 9, 2018
@fm3 fm3 requested a review from jfrohnhofen November 9, 2018 14:14
Copy link
Contributor

@jfrohnhofen jfrohnhofen left a comment

Choose a reason for hiding this comment

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

A few comments - LGTM otherwise.
And could verify the speedups.

implicit val ctx = GlobalAccessContext
logger.info("")
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

val result: Fox[Fox[DataLayer]] = for {
category <- Category.fromString(row.category).toFox ?~> "Could not parse Layer Category"
boundingBox <- BoundingBox
.fromSQL(parseArrayTuple(row.boundingbox).map(_.toInt))
.toFox ?~> "Could not parse boundingbox"
elementClass <- ElementClass.fromString(row.elementclass).toFox ?~> "Could not parse Layer ElementClass"
resolutions <- dataSetResolutionsDAO.findDataResolutionForLayer(dataSetId, row.name) ?~> "Could not find resolution for layer"
standinResolutions: Option[List[Point3D]] = if (skipResolutions) Some(List.empty) else None
resolutions <- Fox.fillOption(standinResolutions)(
Copy link
Contributor

Choose a reason for hiding this comment

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

how about using standinResolutions .getOrElse(...).toFox?

dataSet.logoUrl match {
case Some(url) => Fox.successful(url)
case None => organizationDAO.findOne(dataSet._organization)(GlobalAccessContext).map(_.logoUrl)
case None =>
Fox.fillOption(organization)(organizationDAO.findOne(dataSet._organization)(GlobalAccessContext)).map(_.logoUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

for {
organization <- organizationDAO.findOne(dataSet._organization)(GlobalAccessContext) ?~> "organization.notFound"
dataLayersBox <- dataSetDataLayerDAO.findAllForDataSet(dataSet._id).futureBox
organization <- Fox.fillOption(organization) {
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

for {
teamManagerMemberships <- teamManagerMembershipsFor(user._id)
teamManagerMemberships <- Fox.fillOption(teamMemberships)(teamManagerMembershipsFor(user._id))
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

@@ -150,6 +150,13 @@ object Fox extends FoxImplicits {
Fox.successful(None)}
}

def fillOption[A](input: Option[A])(f: => Fox[A])(implicit ec: ExecutionContext): Fox[A] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove, if Option.getOrElse works in all cases?

val result: Fox[Fox[DataLayer]] = for {
category <- Category.fromString(row.category).toFox ?~> "Could not parse Layer Category"
boundingBox <- BoundingBox
.fromSQL(parseArrayTuple(row.boundingbox).map(_.toInt))
.toFox ?~> "Could not parse boundingbox"
elementClass <- ElementClass.fromString(row.elementclass).toFox ?~> "Could not parse Layer ElementClass"
resolutions <- dataSetResolutionsDAO.findDataResolutionForLayer(dataSetId, row.name) ?~> "Could not find resolution for layer"
standinResolutions: Option[List[Point3D]] = if (skipResolutions) Some(List.empty) else None
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the standinResolutions a bit weird. Can we reduce this to one line? Maybe (not tested):
resolutions <- if (skipResolutions) Fox.successful(Nil) else standinResolutions .getOrElse(...).toFox

@fm3
Copy link
Member Author

fm3 commented Nov 22, 2018

(as discussed in person, the Fox.fillOption cannot be replaced by option.getOrElse because the expected else block has the wrong signature (T instead of Fox[T]))

@fm3 fm3 merged commit 04a4866 into master Nov 22, 2018
@fm3 fm3 deleted the optimize-dataset-list branch November 22, 2018 15:31
jfrohnhofen added a commit that referenced this pull request Nov 23, 2018
* origin/master:
  Optimize performance for the list request /api/datasets (#3441)
  add annotation dataset foreign key  (#3482)
  thumbnails: correctly use zoom value if specified (#3487)
  Store Meshes in Postgres (#3367)
  fix alpha return (#3483)
  Added script to apply all new evolutions (#3427)
  Simple fix to speed up dataset gallery (#3480)
  better errors for screenshot tests, fix imports, refresh screenshots (#3479)
  (Backend only) Add project priority to progress report json (#3476)
  Handle missing write access on datastore (#3411)
  Re-introduce "Flightmode improvements"" (#3473)
  Circleci-notify: linkify PR number (#3469)
  Revert "Flightmode improvements" (#3472)
  also flow-ignore binaryData when using symlinks (#3471)
  Flightmode improvements (#3392)
  Circleci custom notification (#3465)
  enable /api/switch cross-organization (#3464)
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