8145940: TempNewSymbol should have correct copy and assignment functions
Summary: Add clear() to the assignment operator and add copy constructor.
Reviewed-by: mgronlun, lfoltan, kbarrett, jrose
--- a/hotspot/src/share/vm/classfile/symbolTable.cpp Thu Jan 14 17:40:57 2016 +0000
+++ b/hotspot/src/share/vm/classfile/symbolTable.cpp Thu Jan 14 15:45:31 2016 -0500
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -667,3 +667,53 @@
return 0;
}
}
+
+#ifndef PRODUCT
+// Internal test of TempNewSymbol
+void Test_TempNewSymbol() {
+ // Assert messages assume these symbols are unique, and the refcounts start at
+ // one, but code does not rely on this.
+ Thread* THREAD = Thread::current();
+ Symbol* abc = SymbolTable::new_symbol("abc", CATCH);
+ int abccount = abc->refcount();
+ TempNewSymbol ss = abc;
+ assert(ss->refcount() == abccount, "only one abc");
+ assert(ss->refcount() == abc->refcount(), "should match TempNewSymbol");
+
+ Symbol* efg = SymbolTable::new_symbol("efg", CATCH);
+ Symbol* hij = SymbolTable::new_symbol("hij", CATCH);
+ int efgcount = efg->refcount();
+ int hijcount = hij->refcount();
+
+ TempNewSymbol s1 = efg;
+ TempNewSymbol s2 = hij;
+ assert(s1->refcount() == efgcount, "one efg");
+ assert(s2->refcount() == hijcount, "one hij");
+
+ // Assignment operator
+ s1 = s2;
+ assert(hij->refcount() == hijcount + 1, "should be two hij");
+ assert(efg->refcount() == efgcount - 1, "should be no efg");
+
+ s1 = ss; // s1 is abc
+ assert(s1->refcount() == abccount + 1, "should be two abc (s1 and ss)");
+ assert(hij->refcount() == hijcount, "should only have one hij now (s2)");
+
+ s1 = s1; // self assignment
+ assert(s1->refcount() == abccount + 1, "should still be two abc (s1 and ss)");
+
+ TempNewSymbol s3;
+ Symbol* klm = SymbolTable::new_symbol("klm", CATCH);
+ int klmcount = klm->refcount();
+ s3 = klm; // assignment
+ assert(s3->refcount() == klmcount, "only one klm now");
+
+ Symbol* xyz = SymbolTable::new_symbol("xyz", CATCH);
+ int xyzcount = xyz->refcount();
+ { // inner scope
+ TempNewSymbol s_inner = xyz;
+ }
+ assert(xyz->refcount() == (xyzcount - 1),
+ "Should have been decremented by dtor in inner scope");
+}
+#endif // PRODUCT
--- a/hotspot/src/share/vm/classfile/symbolTable.hpp Thu Jan 14 17:40:57 2016 +0000
+++ b/hotspot/src/share/vm/classfile/symbolTable.hpp Thu Jan 14 15:45:31 2016 -0500
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -42,8 +42,17 @@
class BoolObjectClosure;
class outputStream;
-// Class to hold a newly created or referenced Symbol* temporarily in scope.
-// new_symbol() and lookup() will create a Symbol* if not already in the
+// TempNewSymbol acts as a handle class in a handle/body idiom and is
+// responsible for proper resource management of the body (which is a Symbol*).
+// The body is resource managed by a reference counting scheme.
+// TempNewSymbol can therefore be used to properly hold a newly created or referenced
+// Symbol* temporarily in scope.
+//
+// Routines in SymbolTable will initialize the reference count of a Symbol* before
+// it becomes "managed" by TempNewSymbol instances. As a handle class, TempNewSymbol
+// needs to maintain proper reference counting in context of copy semantics.
+//
+// In SymbolTable, new_symbol() and lookup() will create a Symbol* if not already in the
// symbol table and add to the symbol's reference count.
// probe() and lookup_only() will increment the refcount if symbol is found.
class TempNewSymbol : public StackObj {
@@ -51,25 +60,38 @@
public:
TempNewSymbol() : _temp(NULL) {}
- // Creating or looking up a symbol increments the symbol's reference count
+
+ // Conversion from a Symbol* to a TempNewSymbol.
+ // Does not increment the current reference count.
TempNewSymbol(Symbol *s) : _temp(s) {}
- // Operator= increments reference count.
- void operator=(const TempNewSymbol &s) {
- //clear(); //FIXME
- _temp = s._temp;
- if (_temp !=NULL) _temp->increment_refcount();
+ // Copy constructor increments reference count.
+ TempNewSymbol(const TempNewSymbol& rhs) : _temp(rhs._temp) {
+ if (_temp != NULL) {
+ _temp->increment_refcount();
+ }
}
- // Decrement reference counter so it can go away if it's unique
- void clear() { if (_temp != NULL) _temp->decrement_refcount(); _temp = NULL; }
+ // Assignment operator uses a c++ trick called copy and swap idiom.
+ // rhs is passed by value so within the scope of this method it is a copy.
+ // At method exit it contains the former value of _temp, triggering the correct refcount
+ // decrement upon destruction.
+ void operator=(TempNewSymbol rhs) {
+ Symbol* tmp = rhs._temp;
+ rhs._temp = _temp;
+ _temp = tmp;
+ }
- ~TempNewSymbol() { clear(); }
+ // Decrement reference counter so it can go away if it's unused
+ ~TempNewSymbol() {
+ if (_temp != NULL) {
+ _temp->decrement_refcount();
+ }
+ }
- // Operators so they can be used like Symbols
+ // Symbol* conversion operators
Symbol* operator -> () const { return _temp; }
bool operator == (Symbol* o) const { return _temp == o; }
- // Sneaky conversion function
operator Symbol*() { return _temp; }
};
--- a/hotspot/src/share/vm/prims/jni.cpp Thu Jan 14 17:40:57 2016 +0000
+++ b/hotspot/src/share/vm/prims/jni.cpp Thu Jan 14 15:45:31 2016 -0500
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012 Red Hat, Inc.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
@@ -3889,6 +3889,7 @@
void TestResourcehash_test();
void TestChunkedList_test();
void Test_log_length();
+void Test_TempNewSymbol();
#if INCLUDE_ALL_GCS
void TestOldFreeSpaceCalculation_test();
void TestG1BiasedArray_test();
@@ -3932,6 +3933,7 @@
run_unit_test(JSONTest::test());
run_unit_test(Test_log_length());
run_unit_test(DirectivesParser::test());
+ run_unit_test(Test_TempNewSymbol());
#if INCLUDE_VM_STRUCTS
run_unit_test(VMStructs::test());
#endif