Skip to content
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.

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

Move WeakSet to new finalization #13296

Open
wants to merge 6 commits into
base: Pharo11
Choose a base branch
from

Conversation

guillep
Copy link
Member

@guillep guillep commented Apr 5, 2023

Introduce new weakset implementations based on ephemerons

Copy link
Member

@jecisc jecisc left a comment

Choose a reason for hiding this comment

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

The code is much simpler now :)

@jecisc
Copy link
Member

jecisc commented Apr 5, 2023

But the bootstrap does not like it: 

Looking for module  ... loaded...computing accessor depths
warning, variable nsMethodCache doesn't exist or has already been removed
...donecreating initial Objects needed by the VM
Instance of EPRemoteWeakSet did not understand #errorNoFreeSpace
EPRemoteWeakSet(Object)>>doesNotUnderstand: #errorNoFreeSpace
EPRemoteWeakSet>>scanFor:
EPRemoteWeakSet>>like:ifAbsent:
EPInternalSymbolTable>>like:ifAbsent:
[ self like: aLocalSymbol ifAbsent: aBlock ] in EPInternalSymbolTable>>at:ifAbsent: in Block: [ self like: aLocalSymbol ifAbsent: aBlock ]
EPDictionary(Dictionary)>>at:ifAbsent:
EPInternalSymbolTable>>at:ifAbsent:
EPInternalSymbolTable(EPSymbolTable)>>at:ifAbsentPut:
[
		super at: aLocalSymbol ifAbsentPut: aBlock ] in EPInternalSymbolTable>>at:ifAbsentPut: in Block: [...
EPDictionary(Dictionary)>>at:ifAbsent:
EPInternalSymbolTable>>at:ifAbsentPut:
EPObjectSpace>>fromLocalByteSymbol:
ByteSymbol>>asLiteralInObjectSpace:
PBSpurClassLoader(PBClassLoader)>>initializeClassPool:
PBSpurClassLoader(PBClassLoader)>>loadClassNamed:
PBSpurClassLoader(PBClassLoader)>>classNamed:
PBImageBuilderSpur5064bit(PBImageBuilder50)>>classNamed:
PBImageBuilderSpur5064bit(PBImageBuilder50)>>globalNamed:
EPGlobalBinding>>readWith:inNode:
EPASTInterpreter>>visitGlobalVariableNode:
EPGlobalBinding>>acceptVisitor:node:
RBVariableNode>>acceptVisitor:
EPASTInterpreter(ASTInterpreter)>>interpret:
EPASTInterpreter>>visitMessageNode:
RBMessageNode>>acceptVisitor:
EPASTInterpreter(ASTInterpreter)>>interpret:
EPASTInterpreter>>visitAssignmentNode:
RBAssignmentNode>>acceptVisitor:
EPASTInterpreter(ASTInterpreter)>>interpret:
[ :statement|
		lastResult := self interpret: statement.
		self ifSkip: [ ^ lastResult ]] in EPASTInterpreter>>visitSequenceNode: in Block: [ :statement|...

@jecisc jecisc added the Status: Need more work The issue is nearly ready. Waiting some last bits. label Apr 5, 2023

"reclame objects so that slots of ws are nilled out"
o2 := o3 := nil.
2 timesRepeat: [ Smalltalk garbageCollect ].
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this 2 bite use with random CI failures in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully not :)
The thing is: when a single object has many ephemeron subscriptions, only one of them is triggered per GC.
That is because when one ephemeron triggers, it becomes a normal strong object and prevents triggering the other ephemerons (for that same object).

But I'm not against alternatives!! I did this to make the test pass.
Another solution that may be more robust is to GC until the size of the collection is zero.

[ ws isEmpty ] whileFalse: [ Smalltalk garbageCollect ].

This will naturally either:

  • eventually be green
  • or time out for the red

@guillep
Copy link
Member Author

guillep commented Apr 6, 2023

There is a dependency against Espell that assumes the internals of WeakSets to optimise the access to symbol tables.
I'll work it around for now and open an issue.

@jecisc
Copy link
Member

jecisc commented Apr 6, 2023

The fact that it optimize the symbol table is probably the reason why my PR to replace the symbol table by a prefix tree is breaking also :)

@jecisc
Copy link
Member

jecisc commented Apr 6, 2023

There is a new crash in espell:

Halt
EPASTInterpreter(Object)>>halt
EPASTInterpreter>>visitThisContextNode:
ThisContextVariable>>acceptVisitor:node:
RBVariableNode>>acceptVisitor:
EPASTInterpreter(ASTInterpreter)>>interpret:
EPASTInterpreter>>visitMessageNode:
RBMessageNode>>acceptVisitor:
EPASTInterpreter(ASTInterpreter)>>interpret:
EPASTInterpreter>>visitAssignmentNode:
RBAssignmentNode>>acceptVisitor:
EPASTInterpreter(ASTInterpreter)>>interpret:
[ :statement|
		lastResult := self interpret: statement.
		self ifSkip: [ ^ lastResult ]] in EPASTInterpreter>>visitSequenceNode: in Block: [ :statement|...

@guillep guillep changed the title Enh/weakset Move WeakSet to new finalization Apr 6, 2023
@guillep
Copy link
Member Author

guillep commented Apr 6, 2023

@jecisc the cause of this PR failing now is in this line: https://github.com/guillep/espell/blob/063721d51c8a7d6f0bd80ce2f182a9e256c37850/src/Espell.package/EPRemoteWeakSet.class/instance/scanFor..st#L11

The scanFor: method assumes that symbols are raw objects in a weak array.
But this new implementation introduces ephemerons inside a normal array, so there is another indirection.
We need a new release of the bootstrap for this PR to land properly.

GitHub
High-level interface of a pharo runtime. Contribute to guillep/espell development by creating an account on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Need more work The issue is nearly ready. Waiting some last bits.
Projects
Status: NeedsWork/Discussion
Development

Successfully merging this pull request may close these issues.

3 participants