Closed Bug 1651208 Opened 5 months ago Closed 3 months ago

Set a bit in the UsedNameTracker to determine if there are any private names

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox82 --- fixed

People

(Reporter: mgaudet, Assigned: yozaam, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++])

Attachments

(1 file)

For private fields we need to check if there are unbound private names. Right now we do this by iterating over the names, aggregating those that are private + unbound, then figuring out of they have a binding accessible.

This can be optimized by adding a bit to UsedNameTracker to track if we even have seen a private name. If we haven't, then we can bypass these checks. This will be a win avoiding work where it's not necessary.

I want to work on resolving this bug, but this is my first time on Bugzilla and I found this bug using CodeTribute. Could you please initialize me into the system so that I can learn how to contribute?

Flags: needinfo?(mgaudet)

Hi aggarwaluddhav: Apologies for not including this info when I opened the bug. The code I'm asking you to change was added as part of this patch.

Prerequisites

Before getting started, you'll want to

Success Criteria

  • The UsedNameTracker has a new boolean which is set to false at initialization, and true if and only if a private declaration is discovered.
  • That boolean is used to return early from methods that are looking to summarize the private names (ie, one place would be UsedNameTracker::getUnboundPrivateNames
  • After this work, the engine compiles, and passes the tests (run ./mach jit-test )

Questions

If you have questions, feel free to visit us on in the Spidermonkey room on Matrix, or post questions here.

Tips and Tricks:

  • SearchFox indexes the Firefox codebase, and allows you to easily find functions, definitions, and more. It's a powerful tool! Note: at the point I'm submitting the comment, the relevant code has not yet been indexed on searchfox; however that will happen sometime today.
Flags: needinfo?(mgaudet)

I would like to work on this bug. I am a new contributor. I have cloned the repo and learning about the contribution guidelines. Please assign this bug to me. Please let me know about the basic norms about contribution.

Flags: needinfo?(mgaudet)

So, as you can see there's multiple people vyying for this work. The norm here is that we tend not to assign bugs until we have a patch developed (it avoids the case where someone isn't able to complete the work, and so it gets lost).

I would wait a little bit before tackling this, to give aggarwaluddhav a chance, but if you're partially done no worries, feel free to submit a patch.

Flags: needinfo?(mgaudet)

Thanks Matthew! Let me look into this, and will get back after some time.

Priority: -- → P3
Severity: -- → N/A
Assignee: nobody → yozaam
Status: NEW → ASSIGNED

I have tried to add this -> UsedNameTracker class and in the getUnboundPrivateNames hasUnboundPrivateNames

I am sure I have missed a few spots where we need to add it -> I have set it to true when there is visibility private in sed NameTracker::noteUse but I need to still understand this -> where can i set it as true... ? inserting into map_ is what i thought happens there

Also when we delete from the map, i need to remove the private bool to false? so should we change this into a counter?

Counting how many private Names in the usedNameTracker / does delete not take place when going out of scope?

https://phabricator.services.mozilla.com/D88190

Attachment #9172008 - Attachment description: Bug 1651208 put boolean in constructor and get,has functions to track private names in used name tracker r?mgaudet → Bug 1651208 Set a bit in the UsedNameTracker and short circuit some private field early error checks r?mgaudet
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b5c6e21e08f
Set a bit in the UsedNameTracker and short circuit some private field early error checks r=mgaudet
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
You need to log in before you can comment on or make changes to this bug.