Skip to content

scalar: fix the timing issues #2825

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.

Sign up for GitHub

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

Merged
stephanwlee merged 2 commits into from
Oct 24, 2019
Merged

scalar: fix the timing issues #2825

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78c52ab
scalar: fix the timing issues
stephanwlee Oct 23, 2019
69d7233
handle the reset correctly by implementing canceller
stephanwlee Oct 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 77 additions & 43 deletions tensorboard/components/tf_dashboard_common/data-loader-behavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
namespace tf_dashboard_common {
type CacheKey = string;

// NOT_LOADED is implicit
enum LoadState {
LOADING,
LOADED,
}

/**
* @polymerBehavior
*/
Expand Down Expand Up @@ -46,7 +54,7 @@ namespace tf_dashboard_common {
*/
getDataLoadName: {
type: Function,
value: () => (datum) => String(datum),
value: () => (datum): CacheKey => String(datum),
},

/**
Expand Down Expand Up @@ -90,13 +98,13 @@ namespace tf_dashboard_common {
},

/*
* A set of data that is inflight or has been loaded already. This exists
* to prevent fetching same data again.
* A map of a cache key to LoadState. If a cacheKey does not exist in the
* map, it is considered NOT_LOADED.
* Invoking `reload` or a change in `loadKey` clears the cache.
*/
_inflightOrLoadedData: {
_dataLoadState: {
type: Object,
value: () => new Set(),
value: () => new Map<CacheKey, LoadState>(),
},

_canceller: {
Expand All @@ -112,15 +120,16 @@ namespace tf_dashboard_common {
},

reload() {
this._inflightOrLoadedData.clear();
this._dataLoadState.clear();
this._loadData();
},

reset() {
// https://github.com/tensorflow/tensorboard/issues/1499
// Cannot use the observer to observe `loadKey` changes directly.
this.cancelAsync(this._loadDataAsync);
if (this._canceller) this._canceller.cancelAll();
if (this._inflightOrLoadedData) this._inflightOrLoadedData.clear();
if (this._dataLoadState) this._dataLoadState.clear();
if (this.isAttached) this._loadData();
},

Expand All @@ -136,7 +145,13 @@ namespace tf_dashboard_common {
},

detached() {
this._canceller.cancelAll();
// Note: Cannot call canceller.cancelAll since it will poison the cache.
// detached gets called when a component gets unmounted from the document
// but it can be re-mounted. When remounted, poisoned cache will manifest.
// t=0: dataLoadState: 'a' = loading
// t=10: unmount
// t=20: request for 'a' resolves but we do not change the loadState
// because we do not want to set one if, instead, it was resetted at t=10.
this.cancelAsync(this._loadDataAsync);
},

Expand All @@ -149,43 +164,62 @@ namespace tf_dashboard_common {
_loadDataImpl() {
if (!this.active) return;
this.cancelAsync(this._loadDataAsync);
this._loadDataAsync = this.async(() => {
// Read-only property have a special setter.
this._setDataLoading(true);

// Before updating, cancel any network-pending updates, to
// prevent race conditions where older data stomps newer data.
this._canceller.cancelAll();
const promises = this.dataToLoad
.filter((datum) => {
const name = this.getDataLoadName(datum);
return !this._inflightOrLoadedData.has(name);
})
.map((datum) => {
const cacheKey = this.getDataLoadName(datum);
this._inflightOrLoadedData.add(cacheKey);
return this.requestData(datum).then((value) => {
// dataToLoad can change while the request in-flight and it may
// not be no longer required (loadDataCallback can be an expensive
// proess so we would like to omit it if possible).
const isDataRequiredNow = this.dataToLoad.find(
(datum) => this.getDataLoadName(datum) === cacheKey
this._loadDataAsync = this.async(
this._canceller.cancellable((result) => {
if (result.cancelled) {
return;
}
// Read-only property have a special setter.
this._setDataLoading(true);

// Promises return cacheKeys of the data that were fetched.
const promises = this.dataToLoad
.filter((datum) => {
const cacheKey = this.getDataLoadName(datum);
return !this._dataLoadState.has(cacheKey);
})
.map((datum) => {
const cacheKey = this.getDataLoadName(datum);
this._dataLoadState.set(cacheKey, LoadState.LOADING);
return this.requestData(datum).then(
this._canceller.cancellable((result) => {
// It was resetted. Do not notify of the response.
if (!result.cancelled) {
this._dataLoadState.set(cacheKey, LoadState.LOADED);
this.loadDataCallback(this, datum, result.value);
}
return cacheKey;
})
);
if (isDataRequiredNow) {
this.loadDataCallback(this, datum, value);
}
});
});

return Promise.all(promises).then(
this._canceller.cancellable((result) => {
// Read-only property have a special setter.
this._setDataLoading(false);
if (result.cancelled) return;
this.onLoadFinish();
})
);
});

return Promise.all(promises).then(
this._canceller.cancellable((result) => {
// It was resetted. Do not notify of the data load.
if (!result.cancelled) {
Copy link
Contributor

@psybuzz psybuzz Oct 24, 2019

Choose a reason for hiding this comment

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

veeery small nit: I prefer early returns, to prevent deeper indentation

Copy link
Contributor Author

@stephanwlee stephanwlee Oct 24, 2019

Choose a reason for hiding this comment

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

Thanks for the review. Since our CI takes super long and we need this to unblock sync, I will merge as is and we can address this in future time if it truly bothers you.

const keysFetched = result.value;
const fetched = new Set(keysFetched);
const shouldNotify = this.dataToLoad.some((datum) =>
fetched.has(this.getDataLoadName(datum))
);

if (shouldNotify) {
this.onLoadFinish();
}
}

const isDataFetchPending = Array.from(
this._dataLoadState.values()
).some((loadState) => loadState === LoadState.LOADING);

if (!isDataFetchPending) {
// Read-only property have a special setter.
this._setDataLoading(false);
}
})
);
})
);
},
};
} // namespace tf_dashboard_common