[prev in list] [next in list] [prev in thread] [next in thread]
List: webkit-unassigned
Subject: [Webkit-unassigned] [Bug 183888] Add a bisect button to automatically schedule bisecting A/B tasks.
From: bugzilla-daemon () webkit ! org
Date: 2018-03-31 3:36:58
Message-ID: bug-183888-2851-ZistZaIkTc () https ! bugs ! webkit ! org/
[Download RAW message or body]
--Boundary_(ID_TtenVBtWZVF1KpCBRIQQKw)
Date: Fri, 30 Mar 2018 20:37:03 -0700
Auto-submitted: auto-generated
MIME-version: 1.0
Content-type: text/plain; CHARSET=US-ASCII
Content-transfer-encoding: 7BIT
X-Bugzilla-URL: https://bugs.webkit.org/
https://bugs.webkit.org/show_bug.cgi?id=183888
--- Comment #5 from dewei_zhu@apple.com ---
Comment on attachment 336889
--> https://bugs.webkit.org/attachment.cgi?id=336889
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=336889&action=review
> > Websites/perf.webkit.org/public/v3/models/analysis-task.js:175
> > + const allTestGroupsInTask = TestGroup.findAllByTask(this.id());
>
> What guarantees that we've already fetched all the test groups?
It only works for the UI part. I think I need to use fetchForTask instead.
> > Websites/perf.webkit.org/public/v3/models/analysis-task.js:191
> > + try {
>
> I don't really use try-catch elsewhere.
> I'm not certain if it's a good idea to use it here either because it can swallow \
> unexpected exceptions.
When will those error eventually get caught?
> > Websites/perf.webkit.org/public/v3/models/analysis-task.js:210
> > + throw 'Cannot bisect on a range with root/patch/owned commit';
>
> Is the idea that we'd like to implement the MVP and expand it later?
> We certainly want to be able to support this in the future, right?
Yes. Maybe I should abstract all those functions into a module. Essentially what we \
want to do here are: 1. define a bisection range
2. find available commitSets (testable configurations)
3. invoke an algorithm to find the bisection point(s), this algorithm also defines \
what types of commitSet it can process.
For step 2, currently, we only choose the commitSet either exists in \
charts(measurementSet) or test groups (CommitSet), but in the future, it is possible \
that we can get more available binaries. For step 3, current bisection algorithm \
requires CommitSet must not have root/patch/owned commits, but this can be changed if \
more advanced bisection algorithm is developed.
> > Websites/perf.webkit.org/public/v3/models/commit-set.js:169
> > + secondCommitSet.repositories().forEach((repository) => \
> > allRepositories.add(repository));
>
> Why not just new Set([...a, ...b])?
Nice, I didn't know this.
> > Websites/perf.webkit.org/public/v3/models/commit-set.js:186
> > + nameParts.push(`${repository.name()}: Patch-${firstPatch.id()} - \
> > Patch-${secondPatch.id()}`);
>
> I don't think it makes sense to expose the attachment ID like this. We don't do \
> elsewhere in the UI. If we're concerned about the length of the name, then we can \
> just diff the name with the maximum length of name to show. e.g. if we had \
> WebKit-WebComponents-A.patch and WebKit-WebComponents-B.patch, we \
> can just show:
> WebKit: "...A.patch" - "...B.patch"
What if two different patch have the same name?
--
You are receiving this mail because:
You are the assignee for the bug.
--Boundary_(ID_TtenVBtWZVF1KpCBRIQQKw)
Date: Fri, 30 Mar 2018 20:37:04 -0700
Auto-submitted: auto-generated
MIME-version: 1.0
Content-type: text/html; CHARSET=US-ASCII
Content-transfer-encoding: 7BIT
X-Bugzilla-URL: https://bugs.webkit.org/
<html>
<head>
<base href="https://bugs.webkit.org/">
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add a bisect button to automatically schedule bisecting A/B tasks."
href="https://bugs.webkit.org/show_bug.cgi?id=183888#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add a bisect button to automatically schedule bisecting A/B tasks."
href="https://bugs.webkit.org/show_bug.cgi?id=183888">bug 183888</a>
from <span class="vcard"><a class="email" \
href="mailto:dewei_zhu@apple.com" title="dewei_zhu@apple.com">dewei_zhu</a> \
</span></b> <pre>Comment on <span class=""><a \
href="attachment.cgi?id=336889&action=diff" name="attach_336889" \
title="Patch">attachment 336889</a> <a \
href="attachment.cgi?id=336889&action=edit" title="Patch">[details]</a></span> \
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=336889&action=review">https://bugs.webkit.org/attachment.cgi?id=336889&action=review</a>
<span class="quote">>> \
Websites/perf.webkit.org/public/v3/models/analysis-task.js:175 >> + \
const allTestGroupsInTask = TestGroup.findAllByTask(this.id()); >
> What guarantees that we've already fetched all the test groups?</span >
It only works for the UI part. I think I need to use fetchForTask instead.
<span class="quote">>> \
Websites/perf.webkit.org/public/v3/models/analysis-task.js:191 >> + try \
{ >
> I don't really use try-catch elsewhere.
> I'm not certain if it's a good idea to use it here either because it can swallow \
unexpected exceptions.</span >
When will those error eventually get caught?
<span class="quote">>> \
Websites/perf.webkit.org/public/v3/models/analysis-task.js:210 >> + \
throw 'Cannot bisect on a range with root/patch/owned commit'; >
> Is the idea that we'd like to implement the MVP and expand it later?
> We certainly want to be able to support this in the future, right?</span >
Yes. Maybe I should abstract all those functions into a module. Essentially what we \
want to do here are: 1. define a bisection range
2. find available commitSets (testable configurations)
3. invoke an algorithm to find the bisection point(s), this algorithm also defines \
what types of commitSet it can process.
For step 2, currently, we only choose the commitSet either exists in \
charts(measurementSet) or test groups (CommitSet), but in the future, it is possible \
that we can get more available binaries. For step 3, current bisection algorithm \
requires CommitSet must not have root/patch/owned commits, but this can be changed if \
more advanced bisection algorithm is developed.
<span class="quote">>> \
Websites/perf.webkit.org/public/v3/models/commit-set.js:169 >> + \
secondCommitSet.repositories().forEach((repository) => \
allRepositories.add(repository)); >
> Why not just new Set([...a, ...b])?</span >
Nice, I didn't know this.
<span class="quote">>> \
Websites/perf.webkit.org/public/v3/models/commit-set.js:186 >> + \
nameParts.push(`${repository.name()}: Patch-${firstPatch.id()} - \
Patch-${secondPatch.id()}`); >
> I don't think it makes sense to expose the attachment ID like this. We don't do \
elsewhere in the UI. > If we're concerned about the length of the name, then we \
can just diff the name with the maximum length of name to show. > e.g. if we had \
WebKit-WebComponents-A.patch and WebKit-WebComponents-B.patch, we can just show: > \
WebKit: "...A.patch" - "...B.patch"</span >
What if two different patch have the same name?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>
--Boundary_(ID_TtenVBtWZVF1KpCBRIQQKw)--
_______________________________________________
webkit-unassigned mailing list
webkit-unassigned@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-unassigned
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic