Memory Retention: XHR response referenced by Request #8843
Labels
Comments
|
Thanks for reporting and for the detailed write up @bn-dignitas . Would you be able to share a Sandcastle with instructions on reproducing this and if you can indeed see a memory leak from this? You can use one of the 3D Tilesets from the asset depot for testing if you cannot make your own data public. I think this Vricon one is currently the largest available there. |
|
I can reproduce this easy enough with the provided instructions. At first glance it definitely looks like we are holding onto ArrayBuffers because of unreleased XHR requests, but it would be nice to get a second opinion. I suspect you are correct and the Heap is implemented as a circular array so it doesn't automatically clear out old values as much as push them out once some size limit is reached. I don't think I've ever actually touched Heap.js, so @lilleyse or @kring may be able to provide some better insight here. I took a quick look at the "set cancelFunction to undefined" strategy, and that might work assuming there's nothing about the Heap we want to change, but the trick is finding out where we need to put it. Thanks @bn-dignitas! |
Yes this is correct, and is likely the culprit. @bn-dignitas I don't see a problem with the solution you proposed. Would you be willing to open a PR with that fix? |
|
@lilleyse Sure. This would just be a change for the XHR ref and leaving the heap alone, correct? |
|
@bn-dignitas yup, just fixing the KHR ref is fine |
|
@lilleyse I should have noticed that the request.deferred is also referencing the data returned by the XHR response. I think it also needs to be set to undefined, otherwise there is no real effect on memory retention. Does that work for you? Some uncontrolled testing shows decent results in memory retention improvements. The snapshot below shows the top 10 retainers before and after changes. |
|
See #4549 (comment), possibly related |
|
@bn-dignitas are you still working on this? If so, I would recommend just opening a PR with whatever you think is best to do and then we can discuss the details as part of the PR process. Thanks! |
|
@mramato Yes. I got distracted with other priorities, but I'll try to get this in ASAP. |
|
Awesome, thanks! We really appreciate it. |
lilleyse
added a commit
that referenced
this issue
Jun 3, 2020
…ention #8843 Clean up request data references
|
Fixed in #8883 |
katSchmid
added a commit
to PropellerAero/cesium
that referenced
this issue
Jun 24, 2020
* Add translucency rectangle to sandcastle example [skip ci] * Account for primitive restart value. * No const in ES5. * Simplify skinning to always expect VEC4 joint/weight accessors. * Changed code mirror viewport margin to Infinity for improved search * Moves changes to 1.70 * Remove unused imports. * Fix variable and semantic names. * Added frustumSplits option to DebugCameraPrimitive * updated CHANGES.md * Added GlobeTranslucencyState * Fixed ground primitives in orthographic mode * Changed czm_writeDepthClampedToFarPlane to czm_writeDepthClamp and czm_depthClampFarPlane to czm_depthClamp * Add cjs extension to prettier configuration I was modifying gulpfile in another branch and realized formatting was not set up correctly. * Fixed depth plane for orthographic * updated CHANGES.md * Made orthographic depth plane aligned to camera and fixed multifrustum ray ellipsoid intersection * made common variable for camera * Fixed derived commands * Remove invisible translucency state to support classification on invisible surfaces * Fix picking in 2D * Use better image for sandcastle [skip ci] * Don't do depth only shader if clipping planes are enabled [skip ci] * Updated unit tests * Fix a typo * Fix a typo * Render full sky atmosphere when globe is hidden * Add per-fragment atmosphere, improve per-vertex atmosphere * Update CHANGES.md * Fix tests * Add underground color * Add getters/setters, Sandcastle, tests * Update CHANGES.md * Fix CHANGES.md [skip ci] * Added nightAlpha and dayAlpha in the ImageryLayer * Updated CONTRIBUTORS.md * CHANGES.md updated * Added night textures showcase to the Sandcastle * typo * Switch to two-pass classification * Updates from review * Tweak atmosphere under ellipsoid so it's less bright * Fix tests * removed old comment [skip ci] * Distance relative to globe surface * Rename undergroundColorByDistance to undergroundColorAlphaByDistance * Updates from review * Doc [skip ci] * Render underground when clipping planes or cartographic limit are being used * Add czm_backFacing * Only render ground atmosphere for front faces * Show back faces in cartogrphic limit rectangle sandcastle * Fix camera height uniform * Temp * Align ellipsoid geometry to camera * Default perFragmentAtmosphere to false * Support #rgba and #rrggbbaa in Color.fromCssColorString * Updates from review * Same blueness everywhere * Improve alpha * Tweak brightness by fade distance * Revert ground atmosphere change to fix issues when zoomed out really far * Updates from review * Remove old intersection code * Fix lighting crash * Tweak strength to prevent ring artifact in enableLighting mode * Turn off globe lighting in demo * Reorder ifdef's * Reorder updateFrameState * Added GlobeTranslucency to group translucency options * Fix test * Remove frameState from update function * Premultiply alpha for ground polylines * Fix tests * Fix translucencyEnabled not applying when base color alpha is < 1.0 * Define constant once * Add back translucency gradient to sky atmosphere * Add define for globe translucency to SkyAtmosphereCommon * Sky atmosphere fix to not use camera height when intersecting the ellipsoid * some tweaks to day/night blending * Generate official TypeScript type definitions It's been a long requested feature for us to have official TypeScript type definitions. While the community has done a yeoman's job of manually supporting various efforts, the most recent incarnation of which is `@types/cesium`, the sheer scale and ever-evolving nature of Cesium's code base makes manual maintenance a Sisyphean task. Thankfully, our decision to maintain meticulous JSDoc API documentation continues to pay dividends and is what makes automatically generating TypeScript definitions possible. Using the excellent https://github.com/englercj/tsd-jsdoc project we can now automatically generate and even partially validate official definitions as part of the build process. (Thanks to @bampakoa who contributed some early PRs to both CesiumJS and tsd-jsdoc over a year ago and is how I learned about tsd-jsdoc) While tsd-jsdoc output is mostly where we need it to be, we do post-processing on it as well. This lets us clean up the output and also make sure these definitions work whether users include cesium via module, i.e. `import { Cartesian3 } from 'cesium'`, or individual files, i.e. `'import Cartesian3 from 'cesium/Source/Core/Cartesian3'`. There were also some quirks of tsd-jsdoc output we fixed that may eventually turn into a PR into that project from us. The post-processing is part typescript compiler API, part string manipulation. It works and is straightforward but we might want to go full TS api in the future if we decide we need to do more complicated tasks. The output of tsd-jsdoc is currently a little noisy because of some incorrect error reporting, but I'm talking with the maintainer in englercj/tsd-jsdoc#133 to get them fixed. No need to hold up this PR for it though. The definition is generated as a single `Cesium.d.ts` file in Source, so it lives alongside Cesium.js. It is ignored by git but generated by a separate `build-ts` task as part of CI and makeZipFile. This task also validates the file by compiling it with TypeScript, so if a developer does anything too egregious, the build will fail. Definitions are automatically included in our npm packages and release zips and will be automatically used by IDEs thanks to the `types` setting in package.json. This means that IDEs such as VS Code will prefer these types over the existing `@types/cesium` version by default. I didn't want to slow the `build` step down, so I made this a separate step, but in the future we could consider running it by default and we could also unignore this file in Git so that PR reviewers can see the impact, if any, our code changes have on the generated definitions. This might be a good idea as an additional sanity check and should only actually change when the public API itself changes. But the issue would be remembering to run it before submitting the code (or we could use git hooks I suppose?) I just don't want to slow down devs so I'm hesitant to do anything like this out of the gate. We can definitely revisit in the future. A particular exciting thing about this approach is that it exposed a ton of badness in our current JSDoc markup, which is now fixed. Previously, our only concern was "does the doc look right" and we didn't pay attention to whether the meta information generated by JSDoc correctly captured type information (because up until it didn't matter). We leaned particular hard on `@exports` which acted as a catch-all but has now been completely removed from the codebase. All this means is that our documentation as a whole has now improved greatly and will continue to be maintained at this new higher level thanks to incorporating TS definition creation into our pipeline! One minor caveat here is that obviously we changed our JSDoc usage to both make it correct and also accommodate TypeScript. The main drawback to these fixes is that enums are now generated as globals in the doc, rather than modules. This means they no longer have their own dedicated page and are instead on the globals page, but I changed the code to ensure they are still in the table of contents that we generate. I think this trade-off is perfectly fine, but I wanted to mention it since it does change the doc some. We can certainly look into whether we can generate enums on their own page if we think that makes sense. (I actually like this approach a little better personally). Last major piece, the actual code. 99% of the changes in this PR only affect the JSDoc. There are two exceptions: A few of our enums also have private functions tacked onto them. I had to move these functions to be outside the initializer but otherwise they are unchanged. This ensures that a valid TS enum is generated from our code, since you can't have functions globbed onto enums in the TS world. If we were writing TS code by hand, we could use declaration merging with a namespace, but the toolchain we are using doesn't have a way to express that right now. There were two cases where these extra functions weren't private, `ComponentDataType` and `IndexDataType`. That means that as far as the TS definitions goes, the helper methods don't exist. I consder this an edge case and we can write up issues to investigate later. I'm actually not even sure if these functions are public on purposes, @lilleyse can you confirm? We had a few places where we had method signatures with optional parameters that came _before_ required parameters, which is silly. This is invalid TypeScript (and not good API design no matter the language). In 99% of cases this was `equalsEpsilon` style functions where the lhs/rhs were optional but the epsilon was not. I remember the discussion around this when we first did it because we were paranoid about defaulting to 0, but it's an edge case and it's silly so I just changed the epsilon functions to default to zero now, problem solved. Here's a high level summary of the JS changes: * Use proper `@enum` notation instead of `@exports` for enums. * Use proper `@namespace` instead of `@exports` for static classes. * Use proper `@function` instead of `@exports` for standalone functions. * Fix `Promise` markup to actually include the type in all cases, i.e. `Promise` => `Promise<void>` or `Promise<Cartesian3[]>`. * Fix bad markup that referenced types that do not exist (or no longer exist) at the spec level, `Image` => `HTMLImageElement`, `Canvas` => `HTMLCanvasElement`, etc.. `TypedArray` in particular does not exist and much be expressed as a lsit of all applicable types, `Int8Array|Uint8Array|Int16Array|Uint16Array...`. * Use dot notation instead of tilde in callbacks, to better support TypeScript, i.e. `EasingFunction~Callback` becomes `EasingFunction.Callback`. The only exception is when we had a callback type that was i.e. `exportKml~ModelCallback` becomes `exportKmlModelCallback` (a module global type rather than a type on exportKml). This is because it's not possible to have exportKml be both a function and a namespace in this current toolchain. Not a big deal either way since these are meta-types used for defining callbacks but I wanted to mention it. * There were some edge cases where private types that were referenced in the public API but don't exist in the JSDoc. These were trivial to fix by either tweaking the JSDoc to avoid leaking the type or in some cases, just as `PixelDataType`, simply exposing the private type as public. I also found a few cases where things were accidentally public, I marked these as private (these were extreme edge cases so I'm not concerned about breaking changes). Appearances took an optional `RenderState` in their options, I just changed the type to `Object` which we can clean up further later if we need to. * Lots of other little misc JSDoc issues that became obvious once we started to generate definitions (duplicate parameters for example). Thanks again to the community for helping generate ideas and discussions around TS definitions over the last few years and a big thanks to @javagl for helping behind the scenes on this specific effort by evaluating a few different approaches and workaround before we settled on this one (I'm working on a blog with all of the gory details for those interested). Finally, while I'm thrilled with how this turned out (all ~41000 lines and 1.9MB of it), I can guarantee we will uncover some issues with the type definitions as more people use it. The good news is that improving it is now just a matter of fixing the JSDoc, which will benefit the community as a whole and not just TS users. Fixes CesiumGS#2730 Fixes CesiumGS#5717 * Don't hardcode ./node_modules/tsd-jsdoc It may be different in some environments. Instead, just use the normal module name, which will be resolved according to node's module resolution rules. * Use tsconfig.json to avoid errors in some environments. * Add new Sandcastle example. * Add back a "Proxy" type. Since master documented a `Proxy` base class, create it rather than using `DefaultProxy` everywhere. * Add missing `update` function to all DataSource implementations Allows them to be assignable to `DataSource` in the TS definitions. * Minor tweak to doc and Sandcastle * JSDoc fixes to GregorianDate and TimeIntervalCollection * Fix JSDoc for exportKml * Additional JSDoc/TypeScript fixes. * Make IonImageryProvider actually implement all ImageryProvider properties * More TypeScript fixes 1. Make Matrix 2/3/4 ArrayLike objects 2. Make PropertyBag include a string indexor so that it is a dictionary like object. We do this by making up a new private interface and defining it only in the definition file. JSDoc ignores it in the HTML output. * Update CHANGES Fix bad commit. * Number[] -> number[] in TS definition Add workaround for englercj/tsd-jsdoc#117 * Fix Event.raiseEvent TS generation. * Add missing availability property to TerrainProvider classes. * Whoops * More JSDoc fixes and TS improvements 1. Resource has incorrect return types. 2. Improve formatting of TS definition file. * Don't abuse EllipsoidTerrainProvider. * Lots of small changes to improve the typescript definitions. * Give up on trying JSDoc tags for PropertyBag * Example of exporting ConstructorOptions interface * Break out constructor options: Entity and Graphics * Various small type fixes * Selected/tracked entity can be undefined * First pass at generic Events * Constructor interfaces for ImageryProviders * Load-method option interfaces for some DataSources * Options interfaces for Viewer, Resource * CesiumGS#8843 Clear Request cancelFunction and deferred references to free up unintentional retained request data memory * CesiumGS#8843 Add comments * CesiumGS#8843 Update comment * Add CONTRIBUTORS entry * Use function overloading for exportKml * Update CHANGES.md * Update CesiumSandcastle.js * Spell out docs for Resource.QueryParameter * Undo some changes * Add ConstructorOptions to the rest of the ImageryProviders * Workaround CesiumMath -> Math documentation issue * Fix doc links that include a hash. * Improve camera controls when underground * Update CHANGES, tweak doc, and add test. * Fix UrlTemplateImageryProvider Also opt-in tsd-jsdoc so we can use @template in the future. * More fixes found by Kevin. * Tweak CHANGES related to TypeScript * Remove hardcoded node_modules in gulpfile. 1. Use `npx` to execute binaries rather than hardcoded `node_modules/.bin`, which may not always be the right location. 2. Streamline generateDocumentation by calling it sync. * Update CHANGES to make underground visualization more prominent * Fix doc using @member instead of @memberof * Removed duplicate CHANGES.md line * Add Cesium OSM Buildings note in CHANGES * Add Sandcastle link [ci skip] * Fix typo [ci skip] * fixed flipy issue * updated CHANGES.md * added myself to CONTRIBUTORS.md * added tests for flipY function * Fixed Bing Maps Road option in sandcastle example * removed extra whitespace line * fix gulp build error * Added Navagis to CONTRIBUTORS.md * updated tokens, version, and changes.md * Fix variable naming in fog sandcastle * Add toString method for Resource class * Tests for Resource#toString * Use explicit cast * Improve JSDoc/TypeScript support for Material properties All of the MaterialProperty constructors take primitive types in addition to Property instances (and automatically wrap the primitive type in a `ConstantProperty`). The JSDoc was incorrect on this leading to sub-par TS type definitions. Also fixed a missing optional result parameter in the JSDoc for `EllipsoidGeodesic.interpolateUsingSurfaceDistance`. Fixes CesiumGS#8898 * Update CHANGES Add another missing optional result fix. * Update CHANGES for cancelled request memory leak fix * Add additional smokescreen to build-ts 1. Add a Specs/TypeScript directory with a minimal TS configuration that uses the d.ts files generated by build-ts 2. Have `build-ts` compile index.ts from this directory which makes sure various types actually conform to their expected interfaces. 3. Fix ImageryProvider interfaces which had issues exposed by this new test. In the future we can add any additional smokescreens that we think are important to validate our definitions going forward, but this will never be a fully exhaustive check. We currently don't actually execute the output, it's just there for compile-time checking. We can revisit this if we ever feel that it's needed. * Update CHANGES * Add missing property typs to TypeScript smokescreen * Other minor TS improvements. * Make sure all nullable Entity API properties are marked as such. * Most ImageryProvider properties can be undefined. * Fix missing HeadingPitchRoll/HeadingPitchRange JSDoc * Missing `|undefined` in GoogleEarthEnterpriseMapsProvider * Missing JSDoc * change cesiumScriptRegex value fixes Issue CesiumGS#8902 * Expose buildModuleUrl as part of the Cesium API It's been used in Sandcastle examples forever, but has always been marked as private in jsdoc. In order to make it available in TypeScript, we need to expose it. Also cleaned up the doc to be more inline with what the current version actually does. Fixes CesiumGS#8922 * add more test add CesiumSomething test * Correcting Navagis Corporate Link * Two small JSDoc/TS fixes 1. `EllipsoidTangentPlane.fromPoints` takes an array. 2. `EntityCollection.getById` and `CompositeEntityCollection.getById` can both return undefined. * Fix whitespace * Update CHANGES. Also fix missing links from previous PRs * Add some missing readonly documentation tags * Add false as a possible value for skyBox and SkyAtmosphere * CesiumGS#8927: Exposing Transforms.rotationMatrixFromPositionVelocity; Adding tests for Transforms.rotationMatrixFromPositionVelocity * CesiumGS#8927: Adding message for exposing Transforms.rotationMatrixFromPositionVelocity to CHANGES.md * CesiumGS#8927: Running prettier on new rotationMatrixFromPositionVelocity tests * Only include Source folder in TypeScript smokescreen tests. We were including all Cesium directories in the TypeScript smokescreen configuration. This meant that multiple Cesium.d.ts files were included if you did a full build and then ran `build-ts` afterwards. This updates the config used by the smokescreen to only include Source. * Improve base url regex, add additional test. * CesiumGS#8927: Simplifed rotationMatrixFromPositionVelocity tests * Fix geometry creation in TypeScript None of the `XXXGeometry` classes are actually `Geometry` instances. They are instead utility classes that create geometries via their `createGeometry` implementation. `GeometryInstance` can take either "type" but since JS doesn't have types we never really defined what the "utility" type is, so the TypeScript definition for `GeometryInstance` specifies that currently only specifies `Geometry`. This means that this valid JS code is a compiler error in TypeScript ``` const geometryInstance = new GeometryInstance({ geometry: new PolylineGeometry({ positions: [], }), }); ``` To fix this, I introduced a `GeometryFactory` base class like we have elsewhere in the code and changed `GeometryInstance` to take either type. This is the only place where we actually base "non-geometry Geometry" in the API. Happy to consider other names, like `GeometryCreator` if we don't like factory for some reason, but I want to get this in sooner rather than later for 1.70.1 fixes. Also fixed an issue with tsconfig.json I introduced in my last change which was failing to actually catch TS compile errors because it wasn't including the Cesium.d.ts. * Update doc/CHANGES * Bump version and cleanup CHANGES for 1.70.1 release. * adding release 1.70.1 * add codeship * make tests runable * adding propeller code changes back * more changes form diff * Correcting ChANGES.md and index.d.ts Co-authored-by: Matthew Amato <[email protected]> Co-authored-by: Sean Lilley <[email protected]> Co-authored-by: Kevin Ring <[email protected]> Co-authored-by: Ed Mackey <[email protected]> Co-authored-by: Ian Lilley <[email protected]> Co-authored-by: Sam Suhag <[email protected]> Co-authored-by: Sanjeet Suhag <[email protected]> Co-authored-by: Jakub Vrána <[email protected]> Co-authored-by: Edvinas Pranka <[email protected]> Co-authored-by: Omar Shehata <[email protected]> Co-authored-by: Omar Shehata <[email protected]> Co-authored-by: James Bromwell <[email protected]> Co-authored-by: Brandon Nguyen <[email protected]> Co-authored-by: Hannah <[email protected]> Co-authored-by: Eli Bogomolny <[email protected]> Co-authored-by: Peter Gagliardi <[email protected]> Co-authored-by: Jonathan Nogueira <[email protected]> Co-authored-by: xiaobaogeit <[email protected]> Co-authored-by: Jonathan Nogueira <[email protected]> Co-authored-by: Frederic Junod <[email protected]> Co-authored-by: John Remsberg <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
While memory profiling our Cesium based app, I found instances of XHR objects and their response data referenced by the Request cancel function. See below:
In this case, the cancel function added by Response._makeRequest ( https://github.com/CesiumGS/cesium/blob/master/Source/Core/Resource.js#L1347 ) is leaking the xhr object.
This can be seen with any Cesium instance with loaded 3D tiles including the Sandcastle example https://sandcastle.cesium.com/?src=3D%20Tiles%20Interactivity.html
Fly around a bit with Chrome browser and then perform a memory heap snapshot. Look at the system/JSArrayBufferData objects.
Why this may be a problem:
If the xhr response data is referenced by other objects directly and not copied, this would not be a huge deal other than the xhr object should probably be released for GC. However, if this were not always the case, then chunks of memory are being retained inadvertently. I found an instance where the RequestScheduler Heap is referencing Requests for tiles that had been destroyed in our app. This may also be due to the Heap pop function not actually removing Requests out of the array (or maybe that is intentional?), but in any case, if the Request object was not referencing the XHR object then it would not be leaking large amounts of memory (we have some big tiles).
Possible Solution
A quick solution might be to simply delete the cancelFunction in Response._makeRequest when the promise is fulfilled/rejected. I ran a quick test and things seem to work, but I'm not sure if there are any other repercussions.
The text was updated successfully, but these errors were encountered: