Skip to content

Commit

Permalink
Internal: Avoid loading ReviewInfo when PaperInfo not present.
Browse files Browse the repository at this point in the history
And remove some useless invalidate_caches.
  • Loading branch information
kohler committed Feb 22, 2021
1 parent fd7d770 commit c6bafd0
Show file tree
Hide file tree
Showing 10 changed files with 88 additions and 56 deletions.
6 changes: 5 additions & 1 deletion src/conference.php
Original file line number Diff line number Diff line change
Expand Up @@ -953,7 +953,7 @@ function db_error_text($getdb = true) {
function query_error_handler($dblink, $query) {
$landmark = caller_landmark(1, "/^(?:Dbl::|Conf::q|call_user_func)/");
if (PHP_SAPI == "cli") {
fwrite(STDERR, "$landmark: database error: $dblink->error in $query\n");
fwrite(STDERR, "$landmark: database error: $dblink->error in $query\n" . debug_string_backtrace());
} else {
error_log("$landmark: database error: $dblink->error in $query");
self::msg_error("<p>" . htmlspecialchars($landmark) . ": database error: " . htmlspecialchars($this->dblink->error) . " in " . Ht::pre_text_wrap($query) . "</p>");
Expand Down Expand Up @@ -3482,6 +3482,7 @@ function paper_result($options, Contact $user = null) {
// "options"
// "scores" => array(fields to score)
// "assignments"
// "where" => $sql SQL 'where' clause
// "order" => $sql $sql is SQL 'order by' clause (or empty)

$cxid = $user ? $user->contactXid : -2;
Expand Down Expand Up @@ -3669,6 +3670,9 @@ function paper_result($options, Contact $user = null) {
if ($options["myConflicts"] ?? false) {
$where[] = $cxid > 0 ? "PaperConflict.conflictType>" . CONFLICT_MAXUNCONFLICTED : "false";
}
if ($options["where"] ?? false) {
$where[] = $options["where"];
}

$pq = "select " . join(",\n ", $cols)
. "\nfrom " . join("\n ", $joins);
Expand Down
7 changes: 5 additions & 2 deletions src/paperevents.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class PaperEvent {
public $rrow;
/** @var CommentInfo */
public $crow;
/** @var int */
public $eventTime;

/** @param ?ReviewInfo $rrow
Expand All @@ -30,7 +31,9 @@ class PaperEvents {
private $conf;
/** @var Contact */
private $user;
/** @var bool */
private $all_papers = false;
/** @var PaperInfoSet */
private $prows;

private $limit;
Expand Down Expand Up @@ -185,9 +188,9 @@ static function _activity_compar($a, $b) {
} else if (!$a->rrow !== !$b->rrow) {
return $a->rrow ? -1 : 1;
} else if ($a->rrow) {
return $a->rrow->reviewId - $b->rrow->reviewId;
return $a->rrow->reviewId <=> $b->rrow->reviewId;
} else {
return $a->crow->commentId - $b->crow->commentId;
return $a->crow->commentId <=> $b->crow->commentId;
}
}

Expand Down
7 changes: 6 additions & 1 deletion src/paperinfo.php
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ function apply_filter($func) {
}
/** @param callable(PaperInfo):bool $func */
function any($func) {
foreach ($this as $prow) {
foreach ($this->prows as $prow) {
if (($x = call_user_func($func, $prow)))
return $x;
}
Expand All @@ -354,6 +354,11 @@ function offsetSet($offset, $value) {
function offsetUnset($offset) {
throw new Exception("invalid PaperInfoSet::offsetUnset");
}
function ensure_full_reviews() {
if (!empty($this->prows)) {
$this->prows[0]->ensure_full_reviews();
}
}
}

class PaperInfo {
Expand Down
32 changes: 20 additions & 12 deletions src/review.php
Original file line number Diff line number Diff line change
Expand Up @@ -1457,23 +1457,31 @@ function unparse_flow_entry(PaperInfo $prow, ReviewInfo $rrow, Contact $contact)
}

function compute_view_scores() {
$result = $this->conf->qe("select * from PaperReview where reviewViewScore=" . ReviewInfo::VIEWSCORE_RECOMPUTE);
$recompute = $this !== $this->conf->review_form();
$prows = $this->conf->paper_set(["where" => "Paper.paperId in (select paperId from PaperReview where reviewViewScore=" . ReviewInfo::VIEWSCORE_RECOMPUTE . ")"]);
$prows->ensure_full_reviews();
$updatef = Dbl::make_multi_qe_stager($this->conf->dblink);
$pids = $rids = [];
$last_view_score = ReviewInfo::VIEWSCORE_RECOMPUTE;
while (($rrow = ReviewInfo::fetch($result, null, $this->conf, true))) {
$view_score = $this->nonempty_view_score($rrow);
if ($last_view_score !== $view_score) {
if (!empty($rids)) {
$updatef("update PaperReview set reviewViewScore=? where paperId?a and reviewId?a and reviewViewScore=?", [$last_view_score, $pids, $rids, ReviewInfo::VIEWSCORE_RECOMPUTE]);
foreach ($prows as $prow) {
foreach ($prow->reviews_by_id() as $rrow) {
if ($rrow->reviewViewScore_recomputed) {
if ($recompute) {
$rrow->reviewViewScore = $this->nonempty_view_score($rrow);
}
if ($last_view_score !== $rrow->reviewViewScore) {
if (!empty($rids)) {
$updatef("update PaperReview set reviewViewScore=? where paperId?a and reviewId?a and reviewViewScore=?", [$last_view_score, $pids, $rids, ReviewInfo::VIEWSCORE_RECOMPUTE]);
}
$pids = $rids = [];
$last_view_score = $rrow->reviewViewScore;
}
if (empty($pids) || $pids[count($pids) - 1] !== $rrow->paperId) {
$pids[] = $rrow->paperId;
}
$rids[] = $rrow->reviewId;
}
$pids = $rids = [];
$last_view_score = $view_score;
}
if (empty($pids) || $pids[count($pids) - 1] !== $rrow->paperId) {
$pids[] = $rrow->paperId;
}
$rids[] = $rrow->reviewId;
}
if (!empty($rids)) {
$updatef("update PaperReview set reviewViewScore=? where paperId?a and reviewId?a and reviewViewScore=?", [$last_view_score, $pids, $rids, ReviewInfo::VIEWSCORE_RECOMPUTE]);
Expand Down
19 changes: 10 additions & 9 deletions src/reviewinfo.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ class ReviewInfo implements JsonSerializable {
public $reviewNeedsSubmit;
/** @var int */
public $reviewViewScore;
/** @var bool */
public $reviewViewScore_recomputed = false;
/** @var int */
public $reviewStatus;

Expand Down Expand Up @@ -243,8 +245,7 @@ static function make_blank(PaperInfo $prow = null, Contact $user) {
return $rrow;
}

private function merge($recomputing_view_scores, PaperInfo $prow = null,
Conf $conf = null) {
private function merge($is_full, PaperInfo $prow = null, Conf $conf = null) {
$this->conf = $conf ?? $prow->conf;
$this->prow = $prow;
$this->paperId = (int) $this->paperId;
Expand Down Expand Up @@ -342,10 +343,11 @@ private function merge($recomputing_view_scores, PaperInfo $prow = null,
$this->roles = (int) $this->roles;
}

if (!$recomputing_view_scores
&& $this->reviewViewScore == self::VIEWSCORE_RECOMPUTE) {
assert($this->reviewViewScore != self::VIEWSCORE_RECOMPUTE);
$conf->review_form()->compute_view_scores();
if ($this->reviewViewScore == self::VIEWSCORE_RECOMPUTE) {
$this->reviewViewScore_recomputed = true;
if ($is_full) {
$this->reviewViewScore = $conf->review_form()->nonempty_view_score($this);
}
}
}

Expand All @@ -361,12 +363,11 @@ function upgrade_sversion() {
}

/** @return ?ReviewInfo */
static function fetch($result, PaperInfo $prow = null, Conf $conf = null,
$recomputing_view_scores = false) {
static function fetch($result, PaperInfo $prow = null, Conf $conf = null) {
$rrow = $result ? $result->fetch_object("ReviewInfo") : null;
'@phan-var ?ReviewInfo $rrow';
if ($rrow) {
$rrow->merge($recomputing_view_scores, $prow, $conf);
$rrow->merge(true, $prow, $conf);
}
return $rrow;
}
Expand Down
2 changes: 1 addition & 1 deletion src/settings/s_basics.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function validate(SettingValues $sv, Si $si) {
|| $si->name === "conference_name") {
if ($sv->oldv($si->name) !== $sv->newv($si->name)
&& $sv->conf->contactdb()) {
$sv->cleanup_callback("update_shortName", function () use ($sv) {
$sv->register_cleanup_function("update_shortName", function () use ($sv) {
$conf = $sv->conf;
Dbl::ql($conf->contactdb(), "update Conferences set shortName=?, longName=? where dbName=?", $conf->short_name, $conf->long_name, $conf->dbname);
});
Expand Down
2 changes: 1 addition & 1 deletion src/settings/s_options.php
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ function save(SettingValues $sv, Si $si) {
if (!empty($deleted_ids)) {
$sv->conf->qe("delete from PaperOption where optionId?a", $deleted_ids);
}
$sv->mark_invalidate_caches(["options" => true, "autosearch" => true]);
$sv->mark_invalidate_caches(["autosearch" => true]);
}
}
}
59 changes: 32 additions & 27 deletions src/settings/s_reviewform.php
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,32 @@ private function clear_nonexisting_options($fields, Conf $conf) {
return array_keys($updates);
}

static private function compute_review_ordinals(Conf $conf) {
$prows = $conf->paper_set(["where" => "Paper.paperId in (select paperId from PaperReview where reviewOrdinal=0 and reviewSubmitted>0)"]);
$prows->ensure_full_reviews();
$locked = false;
$rf = $conf->review_form();
foreach ($prows as $prow) {
foreach ($prow->reviews_by_id() as $rrow) {
if ($rrow->reviewOrdinal == 0
&& $rrow->reviewSubmitted > 0
&& $rf->nonempty_view_score($rrow) >= VIEWSCORE_AUTHORDEC) {
if (!$locked) {
$conf->qe("lock tables PaperReview write");
$locked = true;
}
$max_ordinal = $conf->fetch_ivalue("select coalesce(max(reviewOrdinal), 0) from PaperReview where paperId=? group by paperId", $rrow->paperId);
if ($max_ordinal !== null) {
$conf->qe("update PaperReview set reviewOrdinal=?, timeDisplayed=? where paperId=? and reviewId=?", $max_ordinal + 1, Conf::$now, $rrow->paperId, $rrow->reviewId);
}
}
}
}
if ($locked) {
$conf->qe("unlock tables");
}
}

function save(SettingValues $sv, Si $si) {
if (!$sv->update("review_form", json_encode_db($this->nrfj))) {
return;
Expand Down Expand Up @@ -340,7 +366,6 @@ function save(SettingValues $sv, Si $si) {
$sv->unset_req($k);
}
}
$sv->conf->invalidate_caches(["rf" => true]);
// reset existing review values
if (!empty($clear_fields)) {
$this->clear_existing_fields($clear_fields, $sv->conf);
Expand All @@ -355,40 +380,20 @@ function save(SettingValues $sv, Si $si) {
}
// assign review ordinals if necessary
if ($assign_ordinal) {
$rrows = [];
$result = $sv->conf->qe("select * from PaperReview where reviewOrdinal=0 and reviewSubmitted>0");
while (($rrow = ReviewInfo::fetch($result, null, $sv->conf))) {
$rrows[] = $rrow;
}
Dbl::free($result);
$locked = false;
foreach ($rrows as $rrow) {
if ($nform->nonempty_view_score($rrow) >= VIEWSCORE_AUTHORDEC) {
if (!$locked) {
$sv->conf->qe("lock tables PaperReview write");
$locked = true;
}
$max_ordinal = $sv->conf->fetch_ivalue("select coalesce(max(reviewOrdinal), 0) from PaperReview where paperId=? group by paperId", $rrow->paperId);
if ($max_ordinal !== null) {
$sv->conf->qe("update PaperReview set reviewOrdinal=?, timeDisplayed=? where paperId=? and reviewId=?", $max_ordinal + 1, Conf::$now, $rrow->paperId, $rrow->reviewId);
}
}
}
if ($locked) {
$sv->conf->qe("unlock tables");
}
$sv->register_cleanup_function("compute_review_ordinals", function () use ($sv) {
self::compute_review_ordinals($sv->conf);
});
}
// reset all word counts if author visibility changed
if ($reset_wordcount) {
$sv->conf->qe("update PaperReview set reviewWordCount=null");
}
// reset all view scores if view scores changed
if ($reset_view_score) {
// XXX should lock out other setting changes
$sv->conf->qe("lock tables PaperReview write");
$sv->conf->qe("update PaperReview set reviewViewScore=" . ReviewInfo::VIEWSCORE_RECOMPUTE);
$nform->compute_view_scores();
$sv->conf->qe("unlock tables");
$sv->register_cleanup_function("compute_review_view_scores", function () use ($sv) {
$sv->conf->review_form()->compute_view_scores();
});
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/settings/s_topics.php
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ function save(SettingValues $sv, Si $si) {
$has_topics = $sv->conf->fetch_ivalue("select exists (select * from TopicArea)");
$sv->save("has_topics", $has_topics ? 1 : null);
$sv->mark_diff("topics");
$sv->mark_invalidate_caches(["topics" => true, "autosearch" => true]);
$sv->mark_invalidate_caches(["autosearch" => true]);
}
}
}
8 changes: 7 additions & 1 deletion src/settingvalues.php
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ function update($name, $value) {
}
/** @param ?string $name
* @param callable() $func */
function cleanup_callback($name, $func) {
function register_cleanup_function($name, $func) {
if ($name !== null) {
foreach ($this->cleanup_callbacks as $cb) {
if ($cb[0] === $name)
Expand All @@ -992,6 +992,12 @@ function cleanup_callback($name, $func) {
}
$this->cleanup_callbacks[] = [$name, $func];
}
/** @param ?string $name
* @param callable() $func
* @deprecated */
function cleanup_callback($name, $func) {
$this->register_cleanup_function($name, $func);
}

/** @param string $field
* @param ?string $classes
Expand Down

0 comments on commit c6bafd0

Please sign in to comment.