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

Merge 2.12.x to 2.13.x [ci: last-only] #8763

Merged
merged 25 commits into from Mar 4, 2020

Conversation

retronym
Copy link
Member

Mostly skipped collections backports.

I skipped the benchmark in 94d8821 as I beleive it overlaps with
RedBlackTreeBenchmark here, but maybe @mkeskells will find part of it
to forward port.

Some Tuple2 avoidance within Map operations from 03387fa is skipped
but is adapted to 2.13.x and spun out to #8761.

S   *   348c541b8a (origin/2.12.x) Merge pull request #8736 from mkeskells/2.12.x_sortedSet-2
S   |\
S   | * e8e5fc8a2f (origin/pr/8736) a simple TreeSetBuilder
S   *   4516efdc5a Merge pull request #8742 from mkeskells/2.12.x_RedBlackBenckmark
S   |\
S   | * 393a0d5ca6 (origin/pr/8742) add more benchmarks for the RedBlackTrees TreeSet and TreeMap
S   | * 94d8821ba1 Basic TreeSet/TreeMap benchmark
S   *   90bc6cf275 Merge pull request #8737 from mkeskells/2.12.x_sortedSet-1
S   |\
S   | * 35c5194dba (origin/pr/8737) if its empty already then there is no need to create more garbage
S   | * 191340fe0e dont create a new TreeSet if there is no change to the underlying tree
S   *   b4c0225e8b Merge pull request #8738 from mkeskells/2.12.x_TreeMap-1
S   |\
S   | * 574758d518 (origin/pr/8738) Dont create a TreeMap if the underlying tree is unchanged
S   *   466f92f504 Merge pull request #8466 from rorygraves/mike/2.12.x_quickHashMap
S   |\
S   | * c0420576fd (rorygraves/mike/2.12.x_quickHashMap, retronym/review/8466, origin/pr/8466, review/8466) add HasForeachEntry
S   | * 4b91530a67 remove a couple of forwarding method
S   | * 03387fa2cb Improve performance of HashMap merge and ++ (mostly).
P   | * 85afb67931 add a benchmark
P   * cf929d678b Merge pull request #8688 from rorygraves/mike/2.12.x_toArray
P   * 6eb8576bb0 (rorygraves/mike/2.12.x_toArray, retronym/topic/review-toArray, origin/pr/8688, topic/review-toArray) Avoid generic array access in loop in ScalaRuntime
P   * e6befa227e Use a while loop in runtime support for varargs array conversions
P   * 2ef4752a7f (mike/2.12.x_toArray) don't create an IntRef in toArray

mkeskells and others added 24 commits February 3, 2020 20:02
Instead, extends the type-case on the src array to separately
deal with each type of primitive array, which can call into
a specialized variant of the loop.

I've also added a special case for empty arrays.

This compiles as:

```
  // access flags 0x1A
  private final static copy$mCc$sp$1([C)[Ljava/lang/Object;
    // parameter final  src
    ALOAD 0
    ARRAYLENGTH
    ISTORE 1
    ILOAD 1
    ICONST_0
    IF_ICMPNE L0
    GETSTATIC scala/Array$.MODULE$ : Lscala/Array$;
    INVOKEVIRTUAL scala/Array$.emptyObjectArray ()[Ljava/lang/Object;
    ARETURN
   L0
    ILOAD 1
    ANEWARRAY java/lang/Object
    ASTORE 2
    ICONST_0
    ISTORE 3
   L1
    ILOAD 3
    ILOAD 1
    IF_ICMPGE L2
    ALOAD 2
    ILOAD 3
    ALOAD 0
    ILOAD 3
    CALOAD
    INVOKESTATIC scala/runtime/BoxesRunTime.boxToCharacter (C)Ljava/lang/Character;
    AASTORE
    ILOAD 3
    ICONST_1
    IADD
    ISTORE 3
    GOTO L1
   L2
    ALOAD 2
    ARETURN
    MAXSTACK = 4
    MAXLOCALS = 4
```
use retainIdentical where appropriate to combine trees
Learning from the optimisations of HashSet bulk operations, port some of that where appropriate to HashMap
 - mostly around HashTrie merge
 - remove more allocations

use System.arraycopy rather then Array.copy for performance
add fast path for some trivial cases, e.g. empty
add fast path to EmptyMap ++ Map, as builders use this path
add allocation testing
add test to ensure simple cases preserve identity on merge
Avoid IntRef and other overheads in `ScalaRuntime.{toArray, toObjectArray}`
[nomerge] optimise the addition of immutable HashMap
dont create a TreeMap if the underlying tree is unchanged
Dont create a new TreeSet if there is no change to the underlying tree
@scala-jenkins scala-jenkins added this to the 2.13.3 milestone Feb 27, 2020
@lrytz
Copy link
Member

lrytz commented Feb 27, 2020

sbt.ForkMain$ForkError: java.lang.AssertionError: assertion failed
	at scala.Predef$.assert(Predef.scala:267)
	at scala.collection.SetMapConsistencyTest.extraMutableAnyRefMapTests(SetMapConsistencyTest.scala:448)

(that test is also updated in https://github.com/scala/scala/pull/8761/files)

@SethTisue SethTisue modified the milestones: 2.13.3, 2.13.2 Mar 3, 2020
@SethTisue SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Mar 3, 2020
This reverts commit 6d49dfd.
@szeiger
Copy link
Member

szeiger commented Mar 3, 2020

Remaining 2.12 changes:

$ git log --oneline --graph 348c541b8a..upstream/2.12.x
*   e1e2bfeaa9 Merge pull request #8778 from retronym/ticket/sd674
|\
| * 98a5944d4d TreeMap is serialization compat with 2.12.10 again
* |   da1bf678e8 Merge pull request #8772 from hrhino/backport/8670
|\ \
| * | d4e90625fb [backport] Build intellij task: don't prompt for confirmation (#8670)
* | |   7ca43d5e7e Merge pull request #8776 from hrhino/topic/align
|\ \ \
| * | | c90c71634f Encourage alignment
| |/ /
* | |   ed743e5be5 Merge pull request #8770 from hrhino/t9908
|\ \ \
| * | | 552510050a Emit advice that compiles
|  / /
* | |   d1e9ac0cc2 Merge pull request #8769 from SethTisue/tweak-8732
|\ \ \
| |/ /
|/| |
| * | 48d3ee23e3 partially revert #8732 to remove problematic isEmpty checks
|/ /
* |   49cec62535 Merge pull request #8764 from retronym/topic/hashmap-merge-regression
|\ \
| |/
|/|
| * cf3a6f1874 Align {add,merge}{Common,Distinct}.
| * 0c62e21014 Fix regression in HashMap.{merge, ++}
|/
*   f139b61e8e Merge pull request #8759 from SethTisue/comment-mini-bootstrap
|\
| * 0469b597b8 improve doc of bootstrapping in PR validation on Travis-CI
*   154215bc18 Merge pull request #8755 from retronym/review/8735
|\
| * 398a891c02 Don't populate Context.implicits for Java sources
| * ca7dc4d597 Use Java semantics for imports in .java files
* ab4d97e87b Merge pull request #8732 from rorygraves/mike/2.12.x_quickTraversable
* 501533adeb remove some refs in TraversableOnce

501533a can be skipped, it no longer applies in 2.13.

@retronym I could use some help with the Context changes in 154215b. importedSelectedSymbol is completely different in 2.13.

@lrytz
Copy link
Member

lrytz commented Mar 3, 2020

@retronym I could use some help with the Context changes in 154215b. importedSelectedSymbol is completely different in 2.13.

If a change is hard to merge 2.12 -> 2.13, I think it's better to skip it (-s ours) and do / ask for a separate PR for 2.13. This makes it easier to review and avoids mistakes.

@retronym
Copy link
Member Author

retronym commented Mar 3, 2020

ask for a separate PR for 2.13

#8784

@SethTisue
Copy link
Member

is this good to go, then?

Copy link
Member

@szeiger szeiger left a comment

Choose a reason for hiding this comment

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

LGTM. The skipped changes are not directly applicable to 2.13.x.

@SethTisue SethTisue merged commit 7da8ce2 into scala:2.13.x Mar 4, 2020
@SethTisue SethTisue removed the prio:blocker release blocker (used only by core team, only near release time) label Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants