Skip to content
Snippets Groups Projects

WIP: Fix setState calls

Closed Lukas Möller requested to merge fix-setState into staging
2 unresolved threads

This branch consists of several changes to this.setState calls. This could allow us to use PureComponent instead of Component which could result in improved performance.

Transformations include:

  • this.setState(prevState => {return {...}}) :arrow_right: this.setState(prevState => ({...}))
  • Better ts types for setState related methods (using keyof Something instead of string) - increases type safety and prevents typos.
  • Using functional logic in react setState closures which is preferred in general (I would suggesting updating typescript so that setState callbacks can benefit of optional chaining i.e. prevState?.sections.map(a => ...) instead of prevState.sections ? prevState.sections.map(a => ...) : undefined)
  • Removing unused parameters
  • Using shorthand object syntax ({a} instead of {a: a})

There are some cases where Map / Set is used which provide no benefit when using them as immutable objects.

Although I am somewhat confident that the transformations are equivalent in terms of the outcome I would be suggest someone else checking this. This merge request is prefixed with WIP until I am certain that the transformations are equivalent and it doesn't introduce any new bugs.

Edited by Lukas Möller

Merge request reports

Pipeline #57989 passed

Pipeline passed for a5990d80 on fix-setState

Closed by Lukas MöllerLukas Möller 4 years ago (Feb 24, 2020 6:05pm UTC)

Merge details

  • The changes were not merged into .

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
274 279 };
275 280
276 281 selectAllExams = (examType: string) => {
277 this.setState(prevState => {
278 prevState.exams.forEach(exam => {
282 this.setState(prevState => ({
283 exams: prevState.exams.map(exam => {
279 284 let currExamtype = exam.examtype ? exam.examtype : "Exams";
280 285 if (currExamtype === examType && exam.canView)
281 286 prevState.selectedExams.add(exam.filename);
  • Lukas Möller
    • Author Contributor

      I would be interested in doing that - a complete rewrite of the split-render code probably wouldn't be a bad idea.

      Some questions to your thoughts: Regarding server side rendering - which format would you render to? I would prefer having an arbitrarily scalable format on the client although rendering to canvas is still bitmap-based (we could however re-render if the user zooms) - also this way dpr can be supported. There is no good pdf to svg renderer available afaik (pdfjs has poor performance) which would mean that bitmap based would be our only option. These image could be larger than transferring pdfjs-dist and the pdf itself (if we render to a big target).

      Regarding the poor performance of the current renderer - I don't have the complete rendering code in my head rn but I thought it would only render the visible splits? I just now noticed it renders the whole page for each split - is that the problem you meant?

    • Author Contributor

      And regarding the use of PureComponent: you are probably correct. I was just changing this because I prefer my state updates like this. You can leave it as it is if that is the way you prefer it.

    • Author Contributor

      Sounds like a good idea.

      • See when and how many times sections are loaded and unloaded
        • => I think it wouldn't be a bad idea to debounce unloading as there are occasions where a user scrolls back and forth while trying to solve an exam which would result in loading and unloading the pdf split/page over and over again. Another idea would be to render splits/pages before they are actually in the viewport, adding a "preloading box" that is slightly bigger than the users viewport
      • Measure how much memory the canvases are using
        • => In !22 (closed) I am adding a force-render option, effectively emulating the "whole exam" strategy. Performance drop is noticeable, after using the feature several times Safari displays a warning along the lines of "This websites impacts your performance...". I think lazy-loading / rendering isn't that bad. However we could implement splitting at several positions in the rendering pipeline. For example instead of effectively rendering the page twice (offscreen and onscreen) we could implement splitting at drawing instruction level, which would mean that we could lazily load and unload every chunk.
      • Test what would happen if you would only load the HTML layer (without the canvas layer) of an exam
        • => Screen_Shot_2020-02-17_at_20.26.45 Screen_Shot_2020-02-17_at_20.25.55
          • Looks kinda weird, some exams certainly cannot be solved in this mode.
          • I think extracting the text stream is only a very small part of rendering the PDF. SSR should be displayed almost instantaneously, but if we still render to canvas it won't really make a difference I think.
          • Memory usage should be a lot better both with SSR and without.
      • Measure time in different parts of the code
        • => Using Chrome Performance analysis tools has shown that pdfjs has the biggest performance impact (both directly from the js and the resulting compositing / sending data to GPU), further investigation might be a good idea.

      Adding a search feature wouldn't be impossible, but there might be issues as its only easy to extract the text-stream of the pdf page, which some pdf producers don't get correct. There is no guarantee that it actually has the correct order, also special characters could be an issue as latex renders ü effectively as u and : which makes German exams hard to search.

      I think if we would want a "low power mode" it might be a better idea to use the pdfjs svg backend. It isn't too bad at rendering text, mathematical equations certainly look fine.

      I also have some other ideas regarding lazy-loading / code-splitting react components which would improve FCP and could potentially save bundle size as some admin-related components are sometimes not required. Currently the main bundle is around 690KB, I got it down to 3 100KB (+ some very small files) bundles (both excluding the pdfjsworker files). I think a decent time of mobile users have to wait a couple of seconds for the page to initially load. Lighthouse score currently is 42 (perf), 78 (accessibility), 93 (best practices), 89 (SEO - shouldn't be important). Although Lighthouse scores might not necessarily reflect our goals it still shows an FCP and FMP of 5.1s (150 ms TCP RTT, 1,638.4 Kbps throughput (Simulated)). A metric which certainly can be improved.

      I think it would be better to discuss performance optimization ideas in a separate issue as it isn't really related to this MR.

    • Please register or sign in to reply
  • Lukas Möller added 2 commits

    added 2 commits

    • 3582913b - Added ts-utils file for utility types
    • 76136959 - Use KeysWithValueType

    Compare with previous version

  • Lukas Möller added 1 commit

    added 1 commit

    Compare with previous version

  • Lukas Möller added 1 commit

    added 1 commit

    Compare with previous version

  • Lukas Möller added 1 commit

    added 1 commit

    • 727c2252 - Finally fixed weird formatting.

    Compare with previous version

  • Lukas Möller added 5 commits

    added 5 commits

    • 050d26dc - Rewritten without modifying prevState.
    • 0d4c0570 - Removed modification of prevState in selectAllExams
    • 45993709 - Refactored unselectAllExams to not modify prevState
    • 68c3703c - Use for of loop instead of copying non-mutating map
    • da70e378 - Made true block look the same as false block

    Compare with previous version

  • Lukas Möller unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Lukas Möller changed the description

    changed the description

  • Lukas Möller added 48 commits

    added 48 commits

    Compare with previous version

  • Lukas Möller added 1 commit

    added 1 commit

    • a5990d80 - Revert "Merge branch 'staging' into 'fix-setState'"

    Compare with previous version

  • Lukas Möller marked as a Work In Progress

    marked as a Work In Progress

  • closed

  • Please register or sign in to reply
    Loading