Skip to content

Conversation

@stephanwlee
Copy link
Contributor

@stephanwlee stephanwlee commented Jan 11, 2021

TensorBoard resources, especially JavaScripts, are not tiny yet they are
not being cached at all by clients. As such, users are downloading large
amount of content every time browser refreshes and it impacts:

  • above the fold time load latency
  • server utilization

This change mitigates above two problems by giving us an ability to
leverage caching by creating a unique resource url with md5 hash of the
file.

Do note that browsers treat, for example, index.js differently than
index.js?random_query.

Internal change: cl/351171360
Test CL: cl/351209032

@google-cla google-cla bot added the cla: yes label Jan 11, 2021
@stephanwlee stephanwlee requested a review from wchargin January 11, 2021 20:37
@stephanwlee stephanwlee marked this pull request as ready for review January 11, 2021 20:37
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I’m okay with a new binary and macro for this. The more we have,
the more I consider defining something like a custom Jinja context with
filters for inlining base64 images, computing file digests, transcluding
files, etc. That way we could have fewer ad hoc genrules everywhere.

@stephanwlee stephanwlee requested a review from wchargin January 12, 2021 19:23
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Lovely; thanks!

@stephanwlee
Copy link
Contributor Author

stephanwlee commented Jan 13, 2021

Moved the macro to tensorboard/defs/internal/resource.bzl.
Rationale: having everything stuffed into defs.bzl or large bzl file makes it harder to do internal transformations so let's break .bzl down?

Please TAL.

@stephanwlee stephanwlee requested a review from wchargin January 13, 2021 19:49
TensorBoard resources, especially JavaScripts, are not tiny yet they are
not being cached at all by clients. As such, users are downloading large
amount of content every time browser refreshes and it impacts:
- above the fold time load latency
- server utilization

This change mitigates above two problems by giving us an ability to
leverage caching by creating a unique resource url with md5 hash of the
file.

Do note that browsers treat, for example, `index.js` differently than
`index.js?random_query`.
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

I don’t entirely understand the internal/external structure—why not move
the contents of defs/internal/resource.bzl to defs/js.bzl?—but no
major objection.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/md5/hash/

@stephanwlee
Copy link
Contributor Author

Any folder with word internal is regarded as an internal set of rules and it disallows other packages like What-If tool to reach in. defs/js.bzl re-exports only what is supposed to be public but in this case, I am not even sure if I want this to be a public rule/macro. Did that address your question?

@wchargin
Copy link
Contributor

Any folder with word internal is regarded as an internal set of rules and it disallows other packages like What-If tool to reach in.

Interesting; I didn’t know this. I don’t entirely understand, since we
load @io_bazel_rules_webtesting//web/internal:platform_http_file.bzl
in our third_party/workspace.bzl. Isn’t that an external package?

I also don’t quite get why it’s valuable to declare this macro in an
internal .bzl file given that we re-export it from js.bzl. Wouldn’t
the What-If Tool be able to access it through js.bzl anyway?

Agreed that it would be nice to avoid random Google packages taking a
dependency on the macro, but my understanding was that this isn’t
possible
. That said, since the expanded macro depends on
:resource_digest_suffixer, does it suffice to limit the visibility of
that binary to //tensorboard:internal (which you’ve done)?

@stephanwlee
Copy link
Contributor Author

Interesting; I didn’t know this. I don’t entirely understand, since we
load @io_bazel_rules_webtesting//web/internal:platform_http_file.bzl
in our third_party/workspace.bzl. Isn’t that an external package?

Technically, it is not a violation at Bazel level but buildozer would scream foul. If you disregard those lint warnings, I guess it has no powers. In other words, it is not Barlog proof.

@wchargin
Copy link
Contributor

I see. In any case, like I said, no major objection; please don’t feel
blocked on my behalf.

@stephanwlee stephanwlee merged commit 8cc6682 into tensorflow:master Jan 15, 2021
@stephanwlee stephanwlee deleted the res_checksum branch January 15, 2021 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants