Port PredictionContext along with tests #36

BurtHarris · 2016-10-06T00:30:41Z

~~Tests all currently fail, remove stubbed out PredictionContextCache and PredictionContext when imports are ready.~~

Edit: Tests are now passing.

sharwell · 2016-10-06T21:09:01Z

I'm planning to add my porting of PredictionContext to this branch so we can review everything at once.

sharwell · 2016-10-08T19:10:43Z

src/atn/PredictionContext.ts

+export abstract class PredictionContext implements Equatable {
+	@NotNull
+	static get EMPTY_LOCAL(): PredictionContext {
+		return PredictionContext.LOAD_LOCAL_CONTEXT();


📝 Need to file an issue to revisit performance of these "constants".

➡️ Filed #58

sharwell · 2016-10-08T19:28:13Z

src/atn/PredictionContext.ts

+
+	private static createSingletonPredictionContext(parent: PredictionContext, returnState: number): PredictionContext {
+		let SingletonPredictionContext: typeof SPC = require('./SingletonPredictionContext');
+		return new SingletonPredictionContext.SingletonPredictionContext(parent, returnState);


📝 TypeScript reports an error on this line, but this form is actually required for the code to work.

sharwell · 2016-10-08T19:41:13Z

src/atn/SingletonPredictionContext.ts

-		assert(returnState != EMPTY_FULL_STATE_KEY && returnState != EMPTY_LOCAL_STATE_KEY);
+	constructor(@NotNull parent: PredictionContext, returnState: number) {
+		super(PredictionContext.calculateSingleHashCode(parent, returnState));
+		// assert(returnState != PredictionContext.EMPTY_FULL_STATE_KEY && returnState != PredictionContext.EMPTY_LOCAL_STATE_KEY);


📝 The assertions in this class are located in performance-critical code.

BurtHarris · 2016-10-08T20:27:41Z

I'm still reviewing to get a better understanding of how it's supposed to work. None the less, with the compile errors it still generates, it doesn't feel solid.

Without fully understanding completely, it still seems to me that this application of optional modules isn't at all what was intended for the feature. I would think it cleaner to package EmptyPredictionContext as a non-exported class in module PredictionContext.ts. It looks like the same could work for ArrayPredictionContext and SingletonPredictionContext as well.

I think that this would allow you to do away with LOAD_FULL_CONTEXT() and LOAD_LOCAL_CONTEXT(), but suggest we do this on a code-clarity basis, rather than just the performance of it.

BurtHarris · 2016-10-08T21:10:58Z

@sharwell, I created branch predictionContext with that refactoring done. Have a look and see what you think. Should be a clean merge into this branch, and reduces the surface area. One trick that let me do this was putting the PredictionContext class at the top of the file, and the PredictionContext namespace at the bottom, so that the other classes were "done" before I needed to use them to initialize the constants.

sharwell · 2016-10-08T21:18:42Z

@BurtHarris Any objection to implementing that as a follow-up to this one (essentially fixing #58)?

BurtHarris · 2016-10-08T21:21:34Z

I guess not, but I'm still reviewing this one. One other thing I noticed was that the Visual Studio project hasn't been updated to include the newly-ported files, but that can wait too.

BurtHarris · 2016-10-08T21:25:43Z

PredictionContext instances are immutable once created, right?

sharwell · 2016-10-08T21:24:58Z

src/misc/Array2DHashMap.ts

+	}
+
+	get(key: K): V {
+		let [k, v] = this.backingStore.get([key, null]) || [undefined, undefined];


💡 It would be better to write this in a way that avoids the fallback allocation. See #59.

sharwell · 2016-10-08T21:25:03Z

src/misc/Array2DHashMap.ts

+	}
+
+	put(key: K, value: V): V {
+		let element: [K, V] = this.backingStore.get([key, value]) || [undefined, undefined];


💡 It would be better to write this in a way that avoids the fallback allocation. See #59.

sharwell · 2016-10-08T21:25:08Z

src/misc/Array2DHashMap.ts

+	}
+
+	putIfAbsent(key: K, value: V): V {
+		let element: [K, V] = this.backingStore.get([key, value]) || [undefined, undefined];


💡 It would be better to write this in a way that avoids the fallback allocation. See #59.

sharwell · 2016-10-08T21:25:59Z

test/tool/TestGraphNodes.ts

@@ -60,7 +68,7 @@ export class TestGraphNodes {
 	}

 	@Test test_$_$_fullctx(): void {
-		let r: PredictionContext =  contextCache.join(PredictionContext.EMPTY_FULL,
+		let r: PredictionContext =  this.contextCache.join(PredictionContext.EMPTY_FULL,
 													  PredictionContext.EMPTY_FULL);
 		console.log(toDOTString(r));


📝 Forgot to comment this console.log out.

sharwell · 2016-10-08T21:28:49Z

PredictionContext instances are immutable once created, right?

They are supposed to be, yes.

BurtHarris · 2016-10-08T21:31:30Z

OK. LGTM. The only thing I might change is declaring the members like cachedHashCode as private readonly. That would flag any violations of the immutability, but that too can be an enhancement. Shall I merge it?

sharwell · 2016-10-08T21:35:18Z

src/atn/ArrayPredictionContext.ts

+	private equalsImpl(other: ArrayPredictionContext, visited: JavaSet<PredictionContextCache.IdentityCommutativePredictionContextOperands>): boolean {
+		let selfWorkList: PredictionContext[] = [];
+		let otherWorkList: PredictionContext[] = [];
+		selfWorkList.unshift(this);


❗ The original code used the work lists as a stack not a queue. This operation should be push so the corresponding pop below is LIFO instead of FIFO.

Good. I'm going to the airport to pick up my daughter who's been traveling in Europe. You can go ahead and merge this when you are done.

sharwell added do not merge in progress labels Oct 6, 2016

Burt Harris and others added 7 commits October 8, 2016 14:00

rename ->TestGraphNodes.ts

78f1401

Adapt TestGraphNodes to Mocha-TypeScript framework

e56ad75

Port PredictionContext and PredictionContextCache

c7577ad

Fix behavior of EMPTY_FULL and EMPTY_LOCAL constants

d0aafe6

Fix imports for TestGraphNodes

a82b50c

Implement prediction context tests in TestGraphNodes

ffc2382

Use Map<K, V> instead of Array2DHashMap<K, V>

17e4743

sharwell force-pushed the testgraphnodes branch from d743f4e to 17e4743 Compare October 8, 2016 19:06

sharwell reviewed Oct 8, 2016

View reviewed changes

sharwell mentioned this pull request Oct 8, 2016

Maximize performance where "optional modules" are used in PredictionContext #58

Closed

sharwell changed the title ~~Port of TestGraphNodes, with stubbed out classes~~ Port PredictionContext along with tests Oct 8, 2016

sharwell added code review do not merge and removed do not merge in progress labels Oct 8, 2016

sharwell assigned sharwell and BurtHarris Oct 8, 2016

sharwell reviewed Oct 8, 2016

View reviewed changes

sharwell added 2 commits October 8, 2016 14:43

Update CONVERSION.md

d11ea55

Convert indentation to tabs

f503ee6

sharwell mentioned this pull request Oct 8, 2016

Avoid unnecessary allocations in Array2DHashMap #59

Closed

sharwell reviewed Oct 8, 2016

View reviewed changes

Comment out remaining call to 'console.log' in TestGraphNodes

478f1c4

Use existing Arrays.binarySearch method

2817121

sharwell reviewed Oct 8, 2016

View reviewed changes

Make the ArrayPredictionContext.equalsImpl work list LIFO

fcb8e07

sharwell removed code review do not merge labels Oct 8, 2016

Make PredictionContext.cachedHashCode readonly

937353a

sharwell merged commit a73faed into master Oct 8, 2016

sharwell deleted the testgraphnodes branch October 8, 2016 21:42

sharwell added enhancement fixed labels Oct 8, 2016

sharwell added needs comment review and removed needs comment review labels Oct 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port PredictionContext along with tests #36

Port PredictionContext along with tests #36

BurtHarris commented Oct 6, 2016 •

edited by sharwell

Loading

sharwell commented Oct 6, 2016

sharwell Oct 8, 2016

sharwell Oct 8, 2016

sharwell Oct 8, 2016

sharwell Oct 8, 2016

BurtHarris commented Oct 8, 2016 •

edited

Loading

BurtHarris commented Oct 8, 2016 •

edited

Loading

sharwell commented Oct 8, 2016

BurtHarris commented Oct 8, 2016

BurtHarris commented Oct 8, 2016

sharwell Oct 8, 2016

sharwell Oct 8, 2016

sharwell Oct 8, 2016

sharwell Oct 8, 2016 •

edited

Loading

sharwell commented Oct 8, 2016

BurtHarris commented Oct 8, 2016 •

edited

Loading

sharwell Oct 8, 2016

BurtHarris Oct 8, 2016

Port PredictionContext along with tests #36

Port PredictionContext along with tests #36

Conversation

BurtHarris commented Oct 6, 2016 • edited by sharwell Loading

sharwell commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurtHarris commented Oct 8, 2016 • edited Loading

BurtHarris commented Oct 8, 2016 • edited Loading

sharwell commented Oct 8, 2016

BurtHarris commented Oct 8, 2016

BurtHarris commented Oct 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

sharwell commented Oct 8, 2016

BurtHarris commented Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurtHarris commented Oct 6, 2016 •

edited by sharwell

Loading

BurtHarris commented Oct 8, 2016 •

edited

Loading

BurtHarris commented Oct 8, 2016 •

edited

Loading

sharwell Oct 8, 2016 •

edited

Loading

BurtHarris commented Oct 8, 2016 •

edited

Loading