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

8248532: Every time I change keyboard language at my MacBook, Java crashes #28

Closed
wants to merge 1 commit into from

Conversation

prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Sep 6, 2020

It seems the observer either needs to be removed when it is dealloced/destroyed (Otherwise Notification Centre would send the notification to the destroyed object resulting in crash.)

and we need to do the below in dealloc, which solves the crash in my system.

NSNotificationCenter *nc = [NSNotificationCenter defaultCenter]; [nc removeObserver:self]

But it is not done for LWCToolkit, so I changed the observer as is being done there, to make existing @implementation class as observer


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8248532: Every time I change keyboard language at my MacBook, Java crashes

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/28/head:pull/28
$ git checkout pull/28

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 6, 2020

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 6, 2020
@openjdk
Copy link

openjdk bot commented Sep 6, 2020

@prsadhuk The following labels will be automatically applied to this pull request: awt. When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

@openjdk openjdk bot added the awt label Sep 6, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 6, 2020

Webrevs

mrserb
mrserb approved these changes Sep 6, 2020
@openjdk
Copy link

openjdk bot commented Sep 6, 2020

@prsadhuk This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8248532: Every time I change keyboard language at my MacBook, Java crashes

Reviewed-by: serb, prr
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 39 commits pushed to the master branch:

  • d560964: 8252794: Creation of JNIMethodBlock should be done with a leaf lock
  • 5fef8dd: 8235229: Compilation against a modular, multi-release JAR erroneous with --release
  • 382b8fe: 8240751: Shenandoah: fold ShenandoahTracer definition
  • c98417e: 8250840: some tests use --enable-preview unnecessarily
  • c655b70: 8252916: Newline in object field values list of ScopeDesc should be removed
  • 30fa8d5: 8157729: examples in LinkedHashMap and LinkedHashSet class doc use raw types
  • 26c7218: 8252773: [TESTBUG] serviceability/jvmti/GetObjectSizeOverflow fails due to OOM conditions
  • e20004d: 8249625: cleanup unused SkippedException in the tests under cds/appcds/dynamicArchive/methodHandles
  • 63a5a12: 8252658: G1: Do not consider G1HeapWastePercent during region selection within a gc
  • 001e51d: 8250563: Add KVHashtable::add_if_absent
  • ... and 29 more: https://git.openjdk.java.net/jdk/compare/ae5a6dde2d77680da5cf4b64c76ab2b4059d3fc4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate d56096471ba042f28c8c2a695c856ca7da156689.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Sep 6, 2020
@prrace
Copy link
Contributor

prrace commented Sep 6, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented Sep 6, 2020

@prrace
The number of required reviews for this PR is now set to 2.

@openjdk openjdk bot removed the ready label Sep 6, 2020
@prrace
Copy link
Contributor

prrace commented Sep 6, 2020

You say you need to use NSDistributedNotificationCenter but you don't. So I am confused.

@prsadhuk
Copy link
Contributor Author

prsadhuk commented Sep 7, 2020

The problem of crash is about not having proper observer. As per
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Notifications/Articles/NotificationCenters.html
NSNotificationCenter delivers notifications to observers synchronously whereas NSDistributedNotificationCenter notification is usually delivered to the main thread’s run loop, thus not depending on observer setup.
Both will work but since we use NSNotificationCenter in mac so far, it seems prudent to use the same methodology for this case too, so I have rectified the observer setup.

@prsadhuk
Copy link
Contributor Author

prsadhuk commented Sep 9, 2020

Hi Phil,
Is there any more feedback on this PR? If not, can you please approve.

@prrace
Copy link
Contributor

prrace commented Sep 9, 2020

You still have not resolved my confusion. Please read your own evaluation in the bug report which says you
will use NSDistributedNotificationCenter.
Is the evaluation wrong ?

@prsadhuk
Copy link
Contributor Author

prsadhuk commented Sep 9, 2020

Yes, the evaluation in the bug report was the first shot on this issue, when I did not know that mac code historically did not use NSDistributedNotificationCenter, rather they use NSNotificationCenter and those did not crash. So, I investigated further to find out it's the observer setup which needs to be amened. I will amend it the evaluation in the bug report. NSDistributedCenter will work but it is more better to use NSNotificationCenter with proper observer setup which I did in 2nd iteration. Hope this clarifies.

prrace
prrace approved these changes Sep 9, 2020
@openjdk openjdk bot added the ready label Sep 9, 2020
@prsadhuk
Copy link
Contributor Author

prsadhuk commented Sep 9, 2020

/integrate

@openjdk openjdk bot closed this Sep 9, 2020
@openjdk openjdk bot added integrated and removed ready labels Sep 9, 2020
@openjdk
Copy link

openjdk bot commented Sep 9, 2020

@prsadhuk Since your change was applied there have been 39 commits pushed to the master branch:

  • d560964: 8252794: Creation of JNIMethodBlock should be done with a leaf lock
  • 5fef8dd: 8235229: Compilation against a modular, multi-release JAR erroneous with --release
  • 382b8fe: 8240751: Shenandoah: fold ShenandoahTracer definition
  • c98417e: 8250840: some tests use --enable-preview unnecessarily
  • c655b70: 8252916: Newline in object field values list of ScopeDesc should be removed
  • 30fa8d5: 8157729: examples in LinkedHashMap and LinkedHashSet class doc use raw types
  • 26c7218: 8252773: [TESTBUG] serviceability/jvmti/GetObjectSizeOverflow fails due to OOM conditions
  • e20004d: 8249625: cleanup unused SkippedException in the tests under cds/appcds/dynamicArchive/methodHandles
  • 63a5a12: 8252658: G1: Do not consider G1HeapWastePercent during region selection within a gc
  • 001e51d: 8250563: Add KVHashtable::add_if_absent
  • ... and 29 more: https://git.openjdk.java.net/jdk/compare/ae5a6dde2d77680da5cf4b64c76ab2b4059d3fc4...master

Your commit was automatically rebased without conflicts.

Pushed as commit 6329de4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@openjdk openjdk bot removed the rfr label Sep 9, 2020
@prsadhuk prsadhuk deleted the 8248532-mackbdcrash branch Sep 9, 2020
cushon pushed a commit to cushon/jdk that referenced this issue Apr 2, 2021
theRealELiu added a commit to theRealELiu/jdk that referenced this issue Mar 29, 2022
This patch optimizes the backend implementation of VectorMaskToLong for
AArch64, given a more efficient approach to mov value bits from
predicate register to general purpose register as x86 PMOVMSK[1] does,
by using BEXT[2] which is available in SVE2.

With this patch, the final code (input mask is byte type with
SPECIESE_512, generated on an SVE vector reg size of 512-bit QEMU
emulator) changes as below:

Before:

        mov     z16.b, p0/z, #1
        fmov    x0, d16
        orr     x0, x0, x0, lsr openjdk#7
        orr     x0, x0, x0, lsr openjdk#14
        orr     x0, x0, x0, lsr openjdk#28
        and     x0, x0, #0xff
        fmov    x8, v16.d[1]
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#8

        orr     x8, xzr, #0x2
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#16

        orr     x8, xzr, #0x3
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#24

        orr     x8, xzr, #0x4
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#32

        mov     x8, #0x5
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#40

        orr     x8, xzr, #0x6
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#48

        orr     x8, xzr, #0x7
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#56

After:

        mov     z16.b, p0/z, #1
        mov     z17.b, #1
        bext    z16.d, z16.d, z17.d
        mov     z17.d, #0
        uzp1    z16.s, z16.s, z17.s
        uzp1    z16.h, z16.h, z17.h
        uzp1    z16.b, z16.b, z17.b
        mov     x0, v16.d[0]

[1] https://www.felixcloutier.com/x86/pmovmskb
[2] https://developer.arm.com/documentation/ddi0602/2020-12/SVE-Instructions/BEXT--Gather-lower-bits-from-positions-selected-by-bitmask-

Change-Id: Ia983a20c89f76403e557ac21328f2f2e05dd08e0
theRealELiu added a commit to theRealELiu/jdk that referenced this issue Apr 21, 2022
This patch optimizes the backend implementation of VectorMaskToLong for
AArch64, given a more efficient approach to mov value bits from
predicate register to general purpose register as x86 PMOVMSK[1] does,
by using BEXT[2] which is available in SVE2.

With this patch, the final code (input mask is byte type with
SPECIESE_512, generated on an SVE vector reg size of 512-bit QEMU
emulator) changes as below:

Before:

        mov     z16.b, p0/z, #1
        fmov    x0, d16
        orr     x0, x0, x0, lsr openjdk#7
        orr     x0, x0, x0, lsr openjdk#14
        orr     x0, x0, x0, lsr openjdk#28
        and     x0, x0, #0xff
        fmov    x8, v16.d[1]
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#8

        orr     x8, xzr, #0x2
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#16

        orr     x8, xzr, #0x3
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#24

        orr     x8, xzr, #0x4
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#32

        mov     x8, #0x5
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#40

        orr     x8, xzr, #0x6
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#48

        orr     x8, xzr, #0x7
        whilele p1.d, xzr, x8
        lastb   x8, p1, z16.d
        orr     x8, x8, x8, lsr openjdk#7
        orr     x8, x8, x8, lsr openjdk#14
        orr     x8, x8, x8, lsr openjdk#28
        and     x8, x8, #0xff
        orr     x0, x0, x8, lsl openjdk#56

After:

        mov     z16.b, p0/z, #1
        mov     z17.b, #1
        bext    z16.d, z16.d, z17.d
        mov     z17.d, #0
        uzp1    z16.s, z16.s, z17.s
        uzp1    z16.h, z16.h, z17.h
        uzp1    z16.b, z16.b, z17.b
        mov     x0, v16.d[0]

[1] https://www.felixcloutier.com/x86/pmovmskb
[2] https://developer.arm.com/documentation/ddi0602/2020-12/SVE-Instructions/BEXT--Gather-lower-bits-from-positions-selected-by-bitmask-

Change-Id: Ia983a20c89f76403e557ac21328f2f2e05dd08e0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awt integrated
3 participants