fixes ort memory leak - #5517

Closed
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak
Closed

fixes ort memory leak#5517
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak

Conversation

@michaelgsharp

@michaelgsharpmichaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

Fixes the ORT memory leak by disposing of the NamedOnnxValues and makes the OnnxRuntimeOutputCacher disposable and adds a finalizer.

@michaelgsharp
michaelgsharp requested a review from a team as a code ownerDecember 1, 2020 18:01
@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run

@azure-pipelines

azure-pipelinesBot commented Dec 1, 2020

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

harishsk
harishsk reviewed Dec 1, 2020
View reviewed changes
Comment threadsrc/Microsoft.ML.OnnxTransformer/OnnxTransform.cs

~OnnxRuntimeOutputCacher()
{
if (_isDisposed)

@harishskharishskDec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it ever hit this code path?

@michaelgsharpmichaelgsharpDec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The finalizer is hit. I dont think the Dispose call is though. I left it in cause it would be better to switch to a dispose call when we are able to.

@eerhardteerhardtDec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't be using a finalizer here.

  1. We should ensure OnnxRuntimeOutputCacher instances get disposed properly.
  2. In the cases where Dispose() is never called by the user, and this object is no longer referenced, all the references to the underlying DisposableNamedOnnxValue will be removed as well. If DisposableNamedOnnxValue is holding on to native memory, it should have a finalizer (or more preferrably use a SafeHandle). The managed object that manages the native memory is responsible for ensuring proper clean up.

We shouldn't be using finalizers to dispose managed objects. Finalizers should only be used for managing native resources.

@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

@eerhardt after syncing with Harish, we are going to go with his approach here. I will be closing this PR.

@harishsk

harishsk commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

@eerhardt Can you please review #5518 instead? We are going with this approach.

@michaelgsharp
michaelgsharp deleted the ort-memory-leak branch March 3, 2021 03:20
@ghostghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@harishskharishskharishsk left review comments
@eerhardteerhardteerhardt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@michaelgsharp@harishsk@eerhardt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fixes ort memory leak - #5517

Closed
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak
Closed

fixes ort memory leak#5517
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak

Conversation

@michaelgsharp

@michaelgsharpmichaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

Fixes the ORT memory leak by disposing of the NamedOnnxValues and makes the OnnxRuntimeOutputCacher disposable and adds a finalizer.

@michaelgsharp
michaelgsharp requested a review from a team as a code ownerDecember 1, 2020 18:01
@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run

@azure-pipelines

azure-pipelinesBot commented Dec 1, 2020

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

harishsk
harishsk reviewed Dec 1, 2020
View reviewed changes
Comment threadsrc/Microsoft.ML.OnnxTransformer/OnnxTransform.cs

~OnnxRuntimeOutputCacher()
{
if (_isDisposed)

@harishskharishskDec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it ever hit this code path?

@michaelgsharpmichaelgsharpDec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The finalizer is hit. I dont think the Dispose call is though. I left it in cause it would be better to switch to a dispose call when we are able to.

@eerhardteerhardtDec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't be using a finalizer here.

  1. We should ensure OnnxRuntimeOutputCacher instances get disposed properly.
  2. In the cases where Dispose() is never called by the user, and this object is no longer referenced, all the references to the underlying DisposableNamedOnnxValue will be removed as well. If DisposableNamedOnnxValue is holding on to native memory, it should have a finalizer (or more preferrably use a SafeHandle). The managed object that manages the native memory is responsible for ensuring proper clean up.

We shouldn't be using finalizers to dispose managed objects. Finalizers should only be used for managing native resources.

@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

@eerhardt after syncing with Harish, we are going to go with his approach here. I will be closing this PR.

@harishsk

harishsk commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

@eerhardt Can you please review #5518 instead? We are going with this approach.

@michaelgsharp
michaelgsharp deleted the ort-memory-leak branch March 3, 2021 03:20
@ghostghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@harishskharishskharishsk left review comments
@eerhardteerhardteerhardt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@michaelgsharp@harishsk@eerhardt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fixes ort memory leak - #5517

Closed
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak
Closed

fixes ort memory leak#5517
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak

Conversation

@michaelgsharp

@michaelgsharpmichaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

Fixes the ORT memory leak by disposing of the NamedOnnxValues and makes the OnnxRuntimeOutputCacher disposable and adds a finalizer.

@michaelgsharp
michaelgsharp requested a review from a team as a code ownerDecember 1, 2020 18:01
@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run

@azure-pipelines

azure-pipelinesBot commented Dec 1, 2020

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

harishsk
harishsk reviewed Dec 1, 2020
View reviewed changes
Comment threadsrc/Microsoft.ML.OnnxTransformer/OnnxTransform.cs

~OnnxRuntimeOutputCacher()
{
if (_isDisposed)

@harishskharishskDec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it ever hit this code path?

@michaelgsharpmichaelgsharpDec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The finalizer is hit. I dont think the Dispose call is though. I left it in cause it would be better to switch to a dispose call when we are able to.

@eerhardteerhardtDec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't be using a finalizer here.

  1. We should ensure OnnxRuntimeOutputCacher instances get disposed properly.
  2. In the cases where Dispose() is never called by the user, and this object is no longer referenced, all the references to the underlying DisposableNamedOnnxValue will be removed as well. If DisposableNamedOnnxValue is holding on to native memory, it should have a finalizer (or more preferrably use a SafeHandle). The managed object that manages the native memory is responsible for ensuring proper clean up.

We shouldn't be using finalizers to dispose managed objects. Finalizers should only be used for managing native resources.

@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

@eerhardt after syncing with Harish, we are going to go with his approach here. I will be closing this PR.

@harishsk

harishsk commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

@eerhardt Can you please review #5518 instead? We are going with this approach.

@michaelgsharp
michaelgsharp deleted the ort-memory-leak branch March 3, 2021 03:20
@ghostghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@harishskharishskharishsk left review comments
@eerhardteerhardteerhardt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@michaelgsharp@harishsk@eerhardt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fixes ort memory leak - #5517

Closed
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak
Closed

fixes ort memory leak#5517
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak

Conversation

@michaelgsharp

@michaelgsharpmichaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

Fixes the ORT memory leak by disposing of the NamedOnnxValues and makes the OnnxRuntimeOutputCacher disposable and adds a finalizer.

@michaelgsharp
michaelgsharp requested a review from a team as a code ownerDecember 1, 2020 18:01
@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run

@azure-pipelines

azure-pipelinesBot commented Dec 1, 2020

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

harishsk
harishsk reviewed Dec 1, 2020
View reviewed changes
Comment threadsrc/Microsoft.ML.OnnxTransformer/OnnxTransform.cs

~OnnxRuntimeOutputCacher()
{
if (_isDisposed)

@harishskharishskDec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it ever hit this code path?

@michaelgsharpmichaelgsharpDec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The finalizer is hit. I dont think the Dispose call is though. I left it in cause it would be better to switch to a dispose call when we are able to.

@eerhardteerhardtDec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't be using a finalizer here.

  1. We should ensure OnnxRuntimeOutputCacher instances get disposed properly.
  2. In the cases where Dispose() is never called by the user, and this object is no longer referenced, all the references to the underlying DisposableNamedOnnxValue will be removed as well. If DisposableNamedOnnxValue is holding on to native memory, it should have a finalizer (or more preferrably use a SafeHandle). The managed object that manages the native memory is responsible for ensuring proper clean up.

We shouldn't be using finalizers to dispose managed objects. Finalizers should only be used for managing native resources.

@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

@eerhardt after syncing with Harish, we are going to go with his approach here. I will be closing this PR.

@harishsk

harishsk commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

@eerhardt Can you please review #5518 instead? We are going with this approach.

@michaelgsharp
michaelgsharp deleted the ort-memory-leak branch March 3, 2021 03:20
@ghostghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@harishskharishskharishsk left review comments
@eerhardteerhardteerhardt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@michaelgsharp@harishsk@eerhardt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fixes ort memory leak - #5517

Closed
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak
Closed

fixes ort memory leak#5517
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak

Conversation

@michaelgsharp

@michaelgsharpmichaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

Fixes the ORT memory leak by disposing of the NamedOnnxValues and makes the OnnxRuntimeOutputCacher disposable and adds a finalizer.

@michaelgsharp
michaelgsharp requested a review from a team as a code ownerDecember 1, 2020 18:01
@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run

@azure-pipelines

azure-pipelinesBot commented Dec 1, 2020

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

harishsk
harishsk reviewed Dec 1, 2020
View reviewed changes
Comment threadsrc/Microsoft.ML.OnnxTransformer/OnnxTransform.cs

~OnnxRuntimeOutputCacher()
{
if (_isDisposed)

@harishskharishskDec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it ever hit this code path?

@michaelgsharpmichaelgsharpDec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The finalizer is hit. I dont think the Dispose call is though. I left it in cause it would be better to switch to a dispose call when we are able to.

@eerhardteerhardtDec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't be using a finalizer here.

  1. We should ensure OnnxRuntimeOutputCacher instances get disposed properly.
  2. In the cases where Dispose() is never called by the user, and this object is no longer referenced, all the references to the underlying DisposableNamedOnnxValue will be removed as well. If DisposableNamedOnnxValue is holding on to native memory, it should have a finalizer (or more preferrably use a SafeHandle). The managed object that manages the native memory is responsible for ensuring proper clean up.

We shouldn't be using finalizers to dispose managed objects. Finalizers should only be used for managing native resources.

@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

@eerhardt after syncing with Harish, we are going to go with his approach here. I will be closing this PR.

@harishsk

harishsk commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

@eerhardt Can you please review #5518 instead? We are going with this approach.

@michaelgsharp
michaelgsharp deleted the ort-memory-leak branch March 3, 2021 03:20
@ghostghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@harishskharishskharishsk left review comments
@eerhardteerhardteerhardt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@michaelgsharp@harishsk@eerhardt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fixes ort memory leak - #5517

Closed
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak
Closed

fixes ort memory leak#5517
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak

Conversation

@michaelgsharp

@michaelgsharpmichaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

Fixes the ORT memory leak by disposing of the NamedOnnxValues and makes the OnnxRuntimeOutputCacher disposable and adds a finalizer.

@michaelgsharp
michaelgsharp requested a review from a team as a code ownerDecember 1, 2020 18:01
@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run

@azure-pipelines

azure-pipelinesBot commented Dec 1, 2020

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

harishsk
harishsk reviewed Dec 1, 2020
View reviewed changes
Comment threadsrc/Microsoft.ML.OnnxTransformer/OnnxTransform.cs

~OnnxRuntimeOutputCacher()
{
if (_isDisposed)

@harishskharishskDec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it ever hit this code path?

@michaelgsharpmichaelgsharpDec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The finalizer is hit. I dont think the Dispose call is though. I left it in cause it would be better to switch to a dispose call when we are able to.

@eerhardteerhardtDec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't be using a finalizer here.

  1. We should ensure OnnxRuntimeOutputCacher instances get disposed properly.
  2. In the cases where Dispose() is never called by the user, and this object is no longer referenced, all the references to the underlying DisposableNamedOnnxValue will be removed as well. If DisposableNamedOnnxValue is holding on to native memory, it should have a finalizer (or more preferrably use a SafeHandle). The managed object that manages the native memory is responsible for ensuring proper clean up.

We shouldn't be using finalizers to dispose managed objects. Finalizers should only be used for managing native resources.

@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

@eerhardt after syncing with Harish, we are going to go with his approach here. I will be closing this PR.

@harishsk

harishsk commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

@eerhardt Can you please review #5518 instead? We are going with this approach.

@michaelgsharp
michaelgsharp deleted the ort-memory-leak branch March 3, 2021 03:20
@ghostghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@harishskharishskharishsk left review comments
@eerhardteerhardteerhardt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@michaelgsharp@harishsk@eerhardt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fixes ort memory leak - #5517

Closed
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak
Closed

fixes ort memory leak#5517
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak

Conversation

@michaelgsharp

@michaelgsharpmichaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

Fixes the ORT memory leak by disposing of the NamedOnnxValues and makes the OnnxRuntimeOutputCacher disposable and adds a finalizer.

@michaelgsharp
michaelgsharp requested a review from a team as a code ownerDecember 1, 2020 18:01
@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run

@azure-pipelines

azure-pipelinesBot commented Dec 1, 2020

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

harishsk
harishsk reviewed Dec 1, 2020
View reviewed changes
Comment threadsrc/Microsoft.ML.OnnxTransformer/OnnxTransform.cs

~OnnxRuntimeOutputCacher()
{
if (_isDisposed)

@harishskharishskDec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it ever hit this code path?

@michaelgsharpmichaelgsharpDec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The finalizer is hit. I dont think the Dispose call is though. I left it in cause it would be better to switch to a dispose call when we are able to.

@eerhardteerhardtDec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't be using a finalizer here.

  1. We should ensure OnnxRuntimeOutputCacher instances get disposed properly.
  2. In the cases where Dispose() is never called by the user, and this object is no longer referenced, all the references to the underlying DisposableNamedOnnxValue will be removed as well. If DisposableNamedOnnxValue is holding on to native memory, it should have a finalizer (or more preferrably use a SafeHandle). The managed object that manages the native memory is responsible for ensuring proper clean up.

We shouldn't be using finalizers to dispose managed objects. Finalizers should only be used for managing native resources.

@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

@eerhardt after syncing with Harish, we are going to go with his approach here. I will be closing this PR.

@harishsk

harishsk commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

@eerhardt Can you please review #5518 instead? We are going with this approach.

@michaelgsharp
michaelgsharp deleted the ort-memory-leak branch March 3, 2021 03:20
@ghostghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@harishskharishskharishsk left review comments
@eerhardteerhardteerhardt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@michaelgsharp@harishsk@eerhardt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fixes ort memory leak - #5517

Closed
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak
Closed

fixes ort memory leak#5517
michaelgsharp wants to merge 1 commit into
dotnet:arcadefrom
michaelgsharp:ort-memory-leak

Conversation

@michaelgsharp

@michaelgsharpmichaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

Fixes the ORT memory leak by disposing of the NamedOnnxValues and makes the OnnxRuntimeOutputCacher disposable and adds a finalizer.

@michaelgsharp
michaelgsharp requested a review from a team as a code ownerDecember 1, 2020 18:01
@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run

@azure-pipelines

azure-pipelinesBot commented Dec 1, 2020

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

harishsk
harishsk reviewed Dec 1, 2020
View reviewed changes
Comment threadsrc/Microsoft.ML.OnnxTransformer/OnnxTransform.cs

~OnnxRuntimeOutputCacher()
{
if (_isDisposed)

@harishskharishskDec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it ever hit this code path?

@michaelgsharpmichaelgsharpDec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The finalizer is hit. I dont think the Dispose call is though. I left it in cause it would be better to switch to a dispose call when we are able to.

@eerhardteerhardtDec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't be using a finalizer here.

  1. We should ensure OnnxRuntimeOutputCacher instances get disposed properly.
  2. In the cases where Dispose() is never called by the user, and this object is no longer referenced, all the references to the underlying DisposableNamedOnnxValue will be removed as well. If DisposableNamedOnnxValue is holding on to native memory, it should have a finalizer (or more preferrably use a SafeHandle). The managed object that manages the native memory is responsible for ensuring proper clean up.

We shouldn't be using finalizers to dispose managed objects. Finalizers should only be used for managing native resources.

@michaelgsharp

michaelgsharp commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

@eerhardt after syncing with Harish, we are going to go with his approach here. I will be closing this PR.

@harishsk

harishsk commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

@eerhardt Can you please review #5518 instead? We are going with this approach.

@michaelgsharp
michaelgsharp deleted the ort-memory-leak branch March 3, 2021 03:20
@ghostghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@harishskharishskharishsk left review comments
@eerhardteerhardteerhardt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@michaelgsharp@harishsk@eerhardt